Memory scoping by iamdefinitelyahuman · Pull Request #2197 · vyperlang/vyper

iamdefinitelyahuman · 2020-10-13T16:28:25Z

What I did

Deallocate internal memory variables between statements.

How I did it

Upon exiting internal_memory_scope, all internal variables declared within the scope are deallocated. The scope is applied prior to parsing each statement node.

How to verify it

Run tests.

Cute Animal Picture

lgtm-com · 2020-10-13T16:47:17Z

This pull request introduces 2 alerts when merging 1ba75c5 into 192d991 - view on LGTM.com

new alerts:

2 for Unguarded next in generator

fubuloubu

Should there be some tests in here?

fubuloubu · 2020-10-13T17:08:29Z

p.s. is there a way to use an infinite generator without lgtm complaining?

codecov-io · 2020-10-14T01:27:55Z

Codecov Report

Merging #2197 into master will decrease coverage by 0.02%.
The diff coverage is 95.06%.

@@            Coverage Diff             @@
##           master    #2197      +/-   ##
==========================================
- Coverage   85.47%   85.45%   -0.03%     
==========================================
  Files          83       83              
  Lines        8503     8511       +8     
  Branches     2053     2057       +4     
==========================================
+ Hits         7268     7273       +5     
- Misses        733      735       +2     
- Partials      502      503       +1

Impacted Files	Coverage Δ
vyper/parser/memory_allocator.py	`83.05% <69.23%> (-5.19%)`	⬇️
vyper/codegen/return_.py	`100.00% <100.00%> (ø)`
vyper/parser/context.py	`96.00% <100.00%> (+0.04%)`	⬆️
vyper/parser/events.py	`93.52% <100.00%> (+0.04%)`	⬆️
...er/function_definitions/parse_external_function.py	`95.78% <100.00%> (ø)`
...yper/parser/function_definitions/parse_function.py	`95.83% <100.00%> (ø)`
...er/function_definitions/parse_internal_function.py	`98.01% <100.00%> (ø)`
vyper/parser/stmt.py	`80.28% <100.00%> (-0.21%)`	⬇️
vyper/signatures/function_signature.py	`81.90% <100.00%> (+0.09%)`	⬆️
vyper/optimizer.py	`84.09% <0.00%> (+0.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 192d991...40b23d5. Read the comment docs.

lgtm-com · 2020-10-14T01:45:05Z

This pull request introduces 2 alerts when merging 40b23d5 into 192d991 - view on LGTM.com

new alerts:

2 for Unguarded next in generator

codecov-commenter · 2020-10-14T01:58:01Z

Codecov Report

Merging #2197 into master will decrease coverage by 0.02%.
The diff coverage is 95.06%.

@@            Coverage Diff             @@
##           master    #2197      +/-   ##
==========================================
- Coverage   85.47%   85.45%   -0.03%     
==========================================
  Files          83       83              
  Lines        8503     8511       +8     
  Branches     2053     2057       +4     
==========================================
+ Hits         7268     7273       +5     
- Misses        733      735       +2     
- Partials      502      503       +1

Impacted Files	Coverage Δ
vyper/parser/memory_allocator.py	`83.05% <69.23%> (-5.19%)`	⬇️
vyper/codegen/return_.py	`100.00% <100.00%> (ø)`
vyper/parser/context.py	`96.00% <100.00%> (+0.04%)`	⬆️
vyper/parser/events.py	`93.52% <100.00%> (+0.04%)`	⬆️
...er/function_definitions/parse_external_function.py	`95.78% <100.00%> (ø)`
...yper/parser/function_definitions/parse_function.py	`95.83% <100.00%> (ø)`
...er/function_definitions/parse_internal_function.py	`98.01% <100.00%> (ø)`
vyper/parser/stmt.py	`80.28% <100.00%> (-0.21%)`	⬇️
vyper/signatures/function_signature.py	`81.90% <100.00%> (+0.09%)`	⬆️
vyper/optimizer.py	`84.09% <0.00%> (+0.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 192d991...d31231c. Read the comment docs.

… for internal vars

codecov-io · 2020-10-14T02:05:02Z

Codecov Report

Merging #2197 into master will decrease coverage by 0.17%.
The diff coverage is 90.56%.

@@            Coverage Diff             @@
##           master    #2197      +/-   ##
==========================================
- Coverage   85.47%   85.30%   -0.18%     
==========================================
  Files          83       83              
  Lines        8503     8524      +21     
  Branches     2053     2061       +8     
==========================================
+ Hits         7268     7271       +3     
- Misses        733      747      +14     
- Partials      502      506       +4

Impacted Files	Coverage Δ
vyper/parser/memory_allocator.py	`63.63% <54.54%> (-24.60%)`	⬇️
vyper/codegen/return_.py	`100.00% <100.00%> (ø)`
vyper/parser/context.py	`96.07% <100.00%> (+0.11%)`	⬆️
vyper/parser/events.py	`93.52% <100.00%> (+0.04%)`	⬆️
...er/function_definitions/parse_external_function.py	`95.78% <100.00%> (ø)`
...yper/parser/function_definitions/parse_function.py	`96.00% <100.00%> (+0.16%)`	⬆️
...er/function_definitions/parse_internal_function.py	`98.01% <100.00%> (ø)`
vyper/parser/self_call.py	`91.93% <100.00%> (+0.19%)`	⬆️
vyper/parser/stmt.py	`80.28% <100.00%> (-0.21%)`	⬇️
vyper/signatures/function_signature.py	`81.90% <100.00%> (+0.09%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 192d991...4685e99. Read the comment docs.

lgtm-com · 2020-10-14T02:17:07Z

This pull request introduces 2 alerts when merging 10a5211 into 192d991 - view on LGTM.com

new alerts:

2 for Unguarded next in generator

lgtm-com · 2020-10-14T19:29:08Z

This pull request introduces 2 alerts when merging 12b388a into 192d991 - view on LGTM.com

new alerts:

2 for Unguarded next in generator

lgtm-com · 2020-10-14T22:22:38Z

This pull request introduces 2 alerts when merging 380cac7 into 192d991 - view on LGTM.com

new alerts:

2 for Unguarded next in generator

fubuloubu · 2020-10-14T23:06:23Z

Shut the hell up lgtm

charles-cooper

this code look really cleaned up compared to before! found a single issue in partially_allocate (the size < self.size caller invariant might change in the future so it's good hygiene to fix that assertion) and some questions / style suggestions.

charles-cooper · 2020-10-21T15:36:50Z

+        int
+            Position of the newly allocated memory
+        """
+        if size > self.size:


this should be >=

Good catch!

charles-cooper · 2020-10-21T15:37:15Z

    def __repr__(self):
        return f"(FreeMemory: pos={self.position}, size={self.size})"

+    def partially_allocate(self, size: int) -> int:


should this be a private function?

charles-cooper · 2020-10-21T15:38:55Z

                active = next_slot
                i += 1
+
+        last = self.deallocated_mem[-1]


could add a comment here

charles-cooper · 2020-10-21T15:40:37Z

+        elif not self.deallocated_mem or self.deallocated_mem[-1].position < pos:
            # no previously deallocated memory, or this is the highest position deallocated
            self.deallocated_mem.append(FreeMemory(position=pos, size=size))
        else:


i feel like instead of maintaining the sort invariant ourselves, we could just make a call to list.sort().

charles-cooper · 2020-10-21T15:42:17Z

+        self.size_of_mem = max(self.size_of_mem, self.next_mem)
        return before_value

    def deallocate_memory(self, pos: int, size: int) -> None:


how about def free()? :)

actually one change i've wanted to make for a long time is to include the units in the name. so, def free_bytes(). makes it easier for the API user to know what is being freed without reading the allocator source.

charles-cooper · 2020-10-21T15:45:14Z

@@ -101,9 +129,8 @@ def deallocate_memory(self, pos: int, size: int) -> None:
        # releasing from the end of the allocated memory - reduce the free memory pointer
        if pos + size == self.next_mem:


i had slight confusion reading this because it overlaps with the final clause in this function. i think this function would read better if we removed this branch.

Yeah, all of this logic can be cleaned up quite a bit.

charles-cooper · 2020-10-21T16:00:03Z


    o.context = context
-    o.total_gas = o.gas + calc_mem_gas(o.context.memory_allocator.get_next_memory_position())
+    o.total_gas = o.gas + calc_mem_gas(o.context.memory_allocator.size_of_mem)


this should be part of the allocator API - allocator.memsize().

charles-cooper · 2020-10-21T16:02:45Z

+        Internal memory scope context manager.
+
+        Internal variables that are declared within this context are de-allocated
+        upon exitting the context.


charles-cooper · 2020-10-21T16:05:18Z

-    def _new_variable(self, name: str, typ: NodeType, var_pos: int) -> int:
+    def _new_variable(self, name: str, typ: NodeType, var_size: int, is_internal: bool) -> int:
+        if is_internal:
+            var_pos = self.memory_allocator.expand_memory(var_size)


why do internal variables get a different allocation strategy?

There are places in parser where an assumption is made that internal variables will be allocated sequentially within memory. If we don't do this, we end up with memory corruption. I tried to unravel / refactor that logic but eventually decided it wasn't safe - I can't be sure I've found them all.

By always allocating new internal variables on the edge of memory like this, we don't risk memory corruption but still gain some efficiency by not expanding memory needlessly. In the future when we refactor parser we should eliminate the assumption about how memory is sequenced, and then we can do away with this logic.

weird. gotcha

charles-cooper · 2020-10-21T16:06:39Z

+            pos=var_pos,
+            typ=typ,
+            mutable=True,
+            blockscopes=self._scopes.copy(),


i'm not sure why a variable needs to have pointers to all parent scopes. this seems like it should just be a single scope_id.

Good point. I just copied this from existing code without thinking 😬

fubuloubu reviewed Oct 13, 2020

View reviewed changes

Comment thread vyper/parser/context.py Outdated

Comment thread vyper/parser/stmt.py

iamdefinitelyahuman force-pushed the feat-mem-scoping branch from 1ba75c5 to 40b23d5 Compare October 14, 2020 01:27

fubuloubu approved these changes Oct 14, 2020

View reviewed changes

Comment thread vyper/parser/function_definitions/parse_function.py Outdated

feat: differentiate between memory allocation and expansion

9df20eb

iamdefinitelyahuman force-pushed the feat-mem-scoping branch from 40b23d5 to d31231c Compare October 14, 2020 01:57

iamdefinitelyahuman added 4 commits October 14, 2020 04:59

feat: internal_memory_scope context manager, reuse for vars, expand…

ed62339

… for internal vars

refactor: call expand_memory instead of allocate_memory

79dd3d4

feat: update context managers

7f9128a

refactor: revert use of new_sequential_variables

1728844

iamdefinitelyahuman force-pushed the feat-mem-scoping branch from d31231c to 10a5211 Compare October 14, 2020 02:00

iamdefinitelyahuman added 6 commits October 14, 2020 21:37

refactor: improve de-allocation logic

0a030bc

refactor: more efficient memory push/pop for maybe-not-sequential memory

0d91d34

test: adjust import style to allow mocking

4536302

test: add mocker for memory var allocation

8094741

test: add memory_mocker fixture to relevent tests

d1d9de4

ci: add memory-mock tests to CI

12b388a

iamdefinitelyahuman force-pushed the feat-mem-scoping branch from 10a5211 to 12b388a Compare October 14, 2020 19:11

fix: track total memory expansion for function gas estimates

380cac7

style: avoid itertools.count because LGTM complains too much

4685e99

fubuloubu approved these changes Oct 14, 2020

View reviewed changes

iamdefinitelyahuman merged commit f0db419 into vyperlang:master Oct 14, 2020

iamdefinitelyahuman deleted the feat-mem-scoping branch October 14, 2020 23:37

charles-cooper reviewed Oct 21, 2020

View reviewed changes

iamdefinitelyahuman mentioned this pull request Oct 23, 2020

Fixes re: memory scoping code review #2204

Merged

		@@ -101,9 +129,8 @@ def deallocate_memory(self, pos: int, size: int) -> None:
		# releasing from the end of the allocated memory - reduce the free memory pointer
		if pos + size == self.next_mem:

Uh oh!

Conversation

iamdefinitelyahuman commented Oct 13, 2020

What I did

How I did it

How to verify it

Cute Animal Picture

Uh oh!

lgtm-com Bot commented Oct 13, 2020

Uh oh!

fubuloubu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fubuloubu commented Oct 13, 2020

Uh oh!

codecov-io commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lgtm-com Bot commented Oct 14, 2020

Uh oh!

Uh oh!

codecov-commenter commented Oct 14, 2020

Codecov Report

Uh oh!

codecov-io commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lgtm-com Bot commented Oct 14, 2020

Uh oh!

lgtm-com Bot commented Oct 14, 2020

Uh oh!

lgtm-com Bot commented Oct 14, 2020

Uh oh!

fubuloubu commented Oct 14, 2020

Uh oh!

charles-cooper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov-io commented Oct 14, 2020 •

edited

Loading

codecov-io commented Oct 14, 2020 •

edited

Loading