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

fix(tracing): reduce stack memory when tracing #5528

Merged
merged 3 commits into from
Nov 22, 2023
Merged

Conversation

gbrew
Copy link
Contributor

@gbrew gbrew commented Nov 22, 2023

A quick and easy fix for #5522 : Keep the trace stacks as a Vec rather than a Stack struct, as the latter always allocates space for a full 1024 stack items. This can add up to many GB of memory when tracing. The cloned Vec will not be fully allocated.

I believe this should not change tracing behavior. When I was checking the parity vmTraces though, it does look like the current "push" stack element reporting is broken, so that's a separate bug that I'll file.

Keep the stack as a Vec<U256> rather than a Stack struct, as the latter
always allocates space for a full 1024 stack items. This can add up to
many GB of memory when tracing.
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

how does this fix the referenced issue?

If I'm reading this correctly, then all this does is moving the wrapped Vec out of the Stack, which has no memory benefits

@mattsse
Copy link
Collaborator

mattsse commented Nov 22, 2023

When I was checking the parity vmTraces though, it does look like the current "push" stack element reporting is broken, so that's a separate bug that I'll file.

please do, that's very likely

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

okay, so even if this does not reduce memory, we should still use the underlying vec

ref bluealloy/revm#876

@mattsse
Copy link
Collaborator

mattsse commented Nov 22, 2023

@gbrew can you please format this?

unable to push because opened from main

@gbrew
Copy link
Contributor Author

gbrew commented Nov 22, 2023

okay, so even if this does not reduce memory, we should still use the underlying vec

ref bluealloy/revm#876

Yeah, I had assumed that Stack::clone() would be fully reserved because it would be memory unsafe if it wasn't.

@gbrew
Copy link
Contributor Author

gbrew commented Nov 22, 2023

@gbrew can you please format this?

unable to push because opened from main

Done, I think? Sorry, I had run the recommended fmt command from Contributing.md ( cargo +nightly fmt -- --check ) before pushing, but it spewed thousands of lines of complaints so I ignored it. I see now that the CI gets it to run clean. Is there some trick to running it?

@mattsse
Copy link
Collaborator

mattsse commented Nov 22, 2023

looks like latest nightly fmt is still broken...

@gbrew
Copy link
Contributor Author

gbrew commented Nov 22, 2023

looks like latest nightly fmt is still broken...

Good to know! Looks like it actually works fine for me on linux but not on macos.

@mattsse
Copy link
Collaborator

mattsse commented Nov 22, 2023

right apparently only macos affected

@mattsse mattsse added this pull request to the merge queue Nov 22, 2023
Merged via the queue into paradigmxyz:main with commit de048c4 Nov 22, 2023
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