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

[4.0 -> main] make eosio_exit impl more direct #995

Merged
merged 3 commits into from
Apr 10, 2023

Conversation

spoonincode
Copy link
Member

main merge of #993

This is simply a clean up change to try and make it more explicit how eosio_exit works by eliminating some needless abstraction that contains a deceptive comment.

When the host function eosio_exit is called the contract execution is considered successfully complete immediately.

I don't remember the exact historical reasons the code was written the way it was, so I'll go with a plausible theory.

A reasonable way to accomplish the required behavior is to throw an exception in the eosio_exit impl that is caught as a do-nothing in the appropriate location (ideally immediately after the line that 'entered' wasm execution). This catch continues to live on

try {
control.get_wasm_interface().apply( receiver_account->code_hash, receiver_account->vm_type, receiver_account->vm_version, *this );
} catch( const wasm_exit& ) {}

However, at some point the performance of this exception was quite poor. Early in EOS' chain history eosio_exit is used a lot. And old EOSIO/Leap implementations had extremely poor exception handling performance when WAVM was being used (the process could spend >50% of its time just handling the eosio_exit host function exceptions!). At some point I did a refactor skipping the exception in WAVM's case to improve performance. Since there isn't an object that represents the "currently running wasm instance", instead wasm_interface gained a new function which purpose was to exit the currently running wasm instance, and then each runtime (like WAVM, etc) could implement this as appropriate.

That's a bad abstraction in the new world of multiple wasm instances executing at the same time.

So this PR restores what was likely similar to the original implementation: just simply throw the exception in eosio_exit host function impl. However, OC continues to use an exception-free optimization for eosio_exit by handling that host function internal to its implementation. So note that on the host function's implementation.

Resolves #956

@linh2931
Copy link
Member

Any reason not to head of main but to GH-980-restore-read-mode-speculative-main?

@spoonincode
Copy link
Member Author

Any reason not to head of main but to GH-980-restore-read-mode-speculative-main?

just stacking up the 4.0->main merges in a clean order

@linh2931
Copy link
Member

Oh, I didn't know this trick. Thanks.

Base automatically changed from GH-980-restore-read-mode-speculative-main to main April 10, 2023 19:32
@spoonincode spoonincode merged commit aad4d4a into main Apr 10, 2023
@spoonincode spoonincode deleted the more_direct_eosio_exit branch April 10, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

investigate "this assumes only one [wasm] can possibly run at a time" comment in context of ROtrx
3 participants