Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce memory usage of tests #1606

Merged
merged 1 commit into from
Dec 17, 2018
Merged

Conversation

lithp
Copy link
Contributor

@lithp lithp commented Dec 14, 2018

@veox pointed out that some of the tests use a very large amount of memory. I did some profiling and found the biggest offenders. Here are the top 10, sorted from least to greatest. This number tracks total allocations (from tracemalloc), not maximum size in memory.

[static_Call50000_identity_d1g0v0_Byzantium] 2.1G
[static_Call50000_identity2_d1g0v0_Byzantium] 2.1G
[static_Call50000_identity2_d1g0v0_Constantinople] 2.1G
[static_Call50000_rip160_d0g0v0_Byzantium] 2.6G
[static_Call50000_rip160_d0g0v0_Constantinople] 2.7G
[static_Call50000_rip160_d1g0v0_Constantinople] 2.7G
[static_Call50000_rip160_d1g0v0_Byzantium] 2.7G
[static_Call50000_ecrec_d1g0v0_Byzantium] 2.8G
[static_Call50000_ecrec_d1g0v0_Constantinople] 2.8G
[static_Call50000_ecrec_d0g0v0_Constantinople] 2.8G

Most allocations came from a single line, which has now been changed:

  • eth/vm/memory.py:Memory.read() used to return a bytes() initialized
    with the requested memory, this created an often unnecessary copy of
    the data. It now returns a memoryview(), which is a python wrapper
    around a pointer to the raw data.

This caused the total number of allocated bytes to drop dramatically. The new set of top 10 offenders:

[static_Call50000_identity2_d1g0v0_Byzantium] 355.9M 373166446
[static_Call50000_rip160_d0g0v0_Byzantium] 440.7M 462087828
[static_Call50000_rip160_d0g0v0_Constantinople] 467.3M 490046338
[static_Call50000_rip160_d1g0v0_Constantinople] 468.5M 491267584
[static_Call50000_rip160_d1g0v0_Byzantium] 468.5M 491271813
[static_Call50000_ecrec_d1g0v0_Constantinople] 485.7M 509305787
[static_Call50000_ecrec_d1g0v0_Byzantium] 486.1M 509705161
[static_Call50000_ecrec_d0g0v0_Constantinople] 486.1M 509709597
[static_Call1MB1024Calldepth_d1g0v0_Byzantium] 539.8M 565999497
[static_Call1MB1024Calldepth_d1g0v0_Constantinople] 539.8M 566007818

Total allocations is not the same as the high water mark of memory used but it was a lot easier for me to measure. To verify that this also helped the thing which actually matters I checked maxrss on some of the tests and saw a nice improvement:

static_Call1MB1024Calldepth_d1g0v0_Constantinople used to consume 1.2 GB and now uses 705 MB
static_Call50000_ecrec_d0g0v0_Constantinople used to consume 3.8GB and now uses 1.4GB

Future work:

  • I didn't give you any help here for verifying the above numbers. It might be useful to also commit some infrastructure for profiling memory usage?

Cute Animal Picture

And alternative approach might be to make all the objects images of kittens, in which case any duplication would remain but gain unassailable justification:

three-kittens

@carver
Copy link
Contributor

carver commented Dec 14, 2018

to verify that this also helped the thing which actually matters I checked maxrss on some of the tests and saw a nice improvement:

static_Call1MB1024Calldepth_d1g0v0_Constantinople used to consume 1.2 GB and now uses 705 MB
static_Call50000_ecrec_d0g0v0_Constantinople used to consume 3.8GB and now uses 1.4GB

This is awesome!

Future work before this can be merged

Which means this should probably be labeled wip and/or renamed with [WIP] as a prefix to the PR title.

  • Maybe there's a superclass of both bytes and memoryview I could use instead of Union[bytes, memoryview].

Doesn't look like it 😞

>>> memoryview.mro()
[memoryview, object]

I guess a custom bytes type we make in eth.typing (and probably eventually eth_typing)

@lithp lithp changed the title Reduce memory usage of tests [WIP] Reduce memory usage of tests Dec 14, 2018
- eth/vm/memory.py:Memory.read() used to return a bytes() initialized
  with the requested memory, this created an often unnecessary copy of
  the data. It now returns a memoryview(), which is a python wrapper
  around a pointer to the raw data.
@lithp
Copy link
Contributor Author

lithp commented Dec 15, 2018

Okay, I've addressed all the things I felt weren't committable and modulo some rate-limiting all the tests have passed. I still don't feel 100% about this code but don't think I'll spend more time on improving it until someone complains :)

@lithp lithp changed the title [WIP] Reduce memory usage of tests Reduce memory usage of tests Dec 15, 2018
Copy link
Contributor

@carver carver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"pending" test is Circle/github rate-limit issue

LGTM!

@lithp lithp merged commit 0a3ac38 into ethereum:master Dec 17, 2018
@lithp lithp deleted the lithp/reduce-memory-usage branch December 17, 2018 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants