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

support for single wasm interface #20

Merged
merged 3 commits into from
Sep 8, 2023
Merged

support for single wasm interface #20

merged 3 commits into from
Sep 8, 2023

Conversation

linh2931
Copy link
Member

Change Description

A part of eosnetworkfoundation/product#149.

This is a support PR for the Leap PR that uses a single WASM interface per nodeos, not per thread. The main change in EOS VM is that compiled module mod is decoupled from backend object.

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

@@ -130,6 +131,16 @@ namespace eosio { namespace vm {
construct();
}

backend& operator=(const backend& other) {
if (this != &other) {
mod = other.mod;
Copy link
Member

Choose a reason for hiding this comment

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

Where is this used. I'm not understanding the use of shared_ptr over unique_ptr and why we need a copy that shares module.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is used https://github.com/AntelopeIO/leap/blob/47e0372ab257534ae5e4133e3e3c6f894569125e/libraries/chain/webassembly/runtimes/eos-vm.cpp#L133. mod stores compiled information about a WASM; it is shared across multiple read-only threads when those threads happen to execute the same contract. The other fields exec_ctx_created_by_backend, initial_max_call_depth, and initial_max_pages are initial values of a backend (instantiated module). They are needed at the start of each execution.

Copy link
Member

Choose a reason for hiding this comment

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

Personally I would prefer a clone() method, or maybe in this case share(). For such a complicated type using operator= in my opinion is too error prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your comments. I was struggling too initially on whether I should implement other constructors.

}
}

void share(const backend& from) {
mod = from.mod;
exec_ctx_created_by_backend = from.exec_ctx_created_by_backend;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right. Nothing is changing ctx here, so exec_ctx_created_by_backend shouldn't change either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you Steve @swatanabe for reviewing this!

The "empty" backend's exec_ctx_created_by_backend is constructed to have the default value true (to support legacy uses). It needs to be set to the actual value from the instantiated module when used the first time.

Copy link
Contributor

Choose a reason for hiding this comment

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

This means that share can only be used in a very specific way. Any other combination will result in leaks or double deletes. This is really dangerous to begin with and it's not even documented or checked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I added checks to prevent invalid sharing and added comments about the method.

@linh2931 linh2931 merged commit b06ac3d into main Sep 8, 2023
10 checks passed
@linh2931 linh2931 deleted the eos_vm_single_wasmif branch September 8, 2023 01:19
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.

4 participants