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

Revamp region system #2137

Merged
merged 11 commits into from May 10, 2024
Merged

Revamp region system #2137

merged 11 commits into from May 10, 2024

Conversation

aumetra
Copy link
Member

@aumetra aumetra commented May 8, 2024

This is a full internal rewrite of the region system, making use of the fact that the stack is part of the linear memory in WASM and can be read by the host.

Meaning a decrease in allocations when calling host functions from the guest module, and the entire system is now a little more robust and has a nicer API.


So far I tested it locally with the crypto-verify contract which is a pretty complex one, but the CI will run all the contracts, so we'll see how it holds up.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Yeah, I think that's an improvment. High level some ideas:

  • Is RegionSource still needed now?
  • A Region makes everything mutable (e.g. there is non-mut [u8] references in the BLS PR, which are written into). Do we need some sort of mutability flag or RwRegion?

packages/vm/src/memory.rs Outdated Show resolved Hide resolved
packages/std/src/memory.rs Show resolved Hide resolved
packages/std/src/memory.rs Outdated Show resolved Hide resolved
packages/std/src/memory.rs Outdated Show resolved Hide resolved
packages/std/src/memory.rs Outdated Show resolved Hide resolved
packages/std/src/imports.rs Outdated Show resolved Hide resolved
packages/std/src/memory.rs Outdated Show resolved Hide resolved
packages/std/src/memory.rs Outdated Show resolved Hide resolved
packages/std/src/imports.rs Outdated Show resolved Hide resolved
@aumetra
Copy link
Member Author

aumetra commented May 8, 2024

It would be fun to see if this actually saves gas in practice, by avoiding calls to the allocator (and potentially even saving memory grow instructions)

@aumetra aumetra requested a review from chipshort May 8, 2024 21:25
Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

LGTM.
In my tests, for a lot of addr_humanize calls, it uses ~30% less gas (comparing inlined String::from_utf8_unchecked vs consume_string_region_written_by_vm), but that number is just for the wasm execution (i.e. ignoring the wasmd part) and could still be off by quite a bit.
I used a cosmwasm_vm::MockApi (so gas cost for the actual call into the chain is comically low compared to what wasmd charges) and there's some overhead for constructing the CanonicalAddr and adding it to the response, the loop itself, etc.
For a real use-case it's probably negligible, since I don't expect people to humanize more than a handful of addresses in one message execution and almost all of the cost for this will be charged by wasmd, but still nice to reduce execution gas cost here.
The change also reduces the wasm size of the addr_humanize function a bit, so that's a nice bonus.

@aumetra aumetra merged commit a35b018 into main May 10, 2024
29 of 30 checks passed
@aumetra aumetra deleted the revamp-regions branch May 10, 2024 11:24
@chipshort
Copy link
Collaborator

As for the gas saving of using stack vs Box pointers: For Storage::set the gas cost is reduced by 85% (similar caveats as above)

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.

None yet

3 participants