Skip to content

Optimize memory representation and operations #707

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

Merged
merged 8 commits into from
Apr 29, 2025
Merged

Conversation

elopez
Copy link
Collaborator

@elopez elopez commented Apr 14, 2025

Description

An Echidna user reported continuously increasing memory while running mstore/mcopy operations. This PR attempts to resolve that by

  • avoiding freezing the full memory on mcopy
  • avoiding multiple, repeated small memory expansions, as each expansion has to copy the whole memory contents
    • this is disabled by default, to maintain the concrete memory small (as it might be symbolized at some later point), but a config value on the VM is available by anyone doing pure concrete execution (e.g. Echidna). Limited empirical testing showed 64k is a good value to use.

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@msooseth
Copy link
Collaborator

Thanks! I am looking at it. Only changing the concrete one is definitely OK. I'm trying to see what's the deal with the Symbolic. Feels like it'd be nice to unify the two, like you did.

@msooseth
Copy link
Collaborator

OK, I had a look. Either way

assign (#state % #memory) (SymbolicMemory $ copySlice srcOff dstOff sz mem mem)

Will get executed. But with your system, we first execute buf <- readMemory srcOff sz, which means:

copySlice offset' (Lit 0) size' mem mempty

which is basically another copyslice. So I'd rather keep the concrete and the symbolic separate.

Otherwise, it looks cool!

This might be desirable to keep Concrete turned Symbolic memories small
@elopez elopez changed the title mcopy: simplify operation, avoid freezing whole memory Optimize memory representation and operations Apr 15, 2025
@elopez elopez requested a review from msooseth April 16, 2025 07:25
@msooseth
Copy link
Collaborator

Hey! If you could remove that tunable then I think we could just merge :) Would be nice!

@msooseth
Copy link
Collaborator

Thanks, this is great! Can you please add a CHANGELOG element? Then I think we can merge :)

@elopez elopez marked this pull request as ready for review April 27, 2025 20:46
@elopez elopez requested a review from msooseth April 27, 2025 20:47
@msooseth msooseth merged commit 6d79cd5 into main Apr 29, 2025
8 checks passed
@msooseth msooseth deleted the dev-mcopy-leak branch April 29, 2025 08:45
@msooseth
Copy link
Collaborator

Thanks! I squash-merged, so it's a single commit. I hope that's OK!

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