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

Allow Wasm module instantiation in host functions called from Wasmi's executor #1116

Merged
merged 11 commits into from
Jul 7, 2024

Conversation

Robbepop
Copy link
Member

@Robbepop Robbepop commented Jul 7, 2024

So far it was not possible to instantiate a Wasm module in a host function that has been called from Wasmi's executor.
This was because of a dead lock that occurred due to a RwLock<EngineResources> which contained the FuncTypeRegistry which was (in the end) mainly used for resolving the number of parameter and result types of a called host function in the Wasmi executor.

There are two ways of solving this with the following change:

Remove EngineResources and instead RwLock both CodeMap and FuncTypeRegistry themselves.

  1. Continue to use &CodeMap in the Wasmi executor but now also a RwLock<FuncTypeRegistry>. Unlock the registry very shortly to resolve the number of parameters and results whenever a host function is called which should occur kind of rarely. The downside is that this probably regresses performance of host function calls due to the additional locking per call.
  2. Populate HostFuncEntity with inline information about the number of its parameter types and result types. This acts as chace. Since HostFuncEntity has to be resolved anyway for a host function call no performance regressions are expected with this solution. The downside is increased space requirements for the FuncEntity buffer.

This PR chose 2. for its implementation. Before merging we might want to experiment with another PR that tried out 1. Local benchmarks confirmed that there were no significant changes.

ToDo

  • Try to reduce space regression by using u16 instead of usize to store the inline number of parameters and results in HostFuncEntity. This is safely possible since we limit the number of both values between 0..=1000 similar to Wasmtime.

Robbepop added 4 commits July 7, 2024 12:33
This test currently dead locks as expected.
This finally resolves the dead lock when instantiating a Wasm module in a called host function.
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

Attention: Patch coverage is 83.63636% with 18 lines in your changes missing coverage. Please review.

Project coverage is 80.51%. Comparing base (dd16c12) to head (7590b83).

Files Patch % Lines
...ates/wasmi/tests/e2e/v1/host_call_instantiation.rs 71.05% 11 Missing ⚠️
crates/wasmi/src/engine/executor/mod.rs 80.00% 3 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/call.rs 83.33% 2 Missing ⚠️
crates/wasmi/src/engine/executor/instrs.rs 0.00% 1 Missing ⚠️
crates/wasmi/src/engine/mod.rs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1116      +/-   ##
==========================================
+ Coverage   80.48%   80.51%   +0.03%     
==========================================
  Files         270      271       +1     
  Lines       25079    25110      +31     
==========================================
+ Hits        20184    20217      +33     
+ Misses       4895     4893       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Robbepop added 7 commits July 7, 2024 22:11
New enforced maximum parameter and result types per FuncType is 1000. This fits nicely in a u16 value which we will make use of later in the API.
This also make use of u16 in HostFuncEntity to inline cache len_{params,results} as an space optimization.
@Robbepop
Copy link
Member Author

Robbepop commented Jul 7, 2024

I have now refactored FuncType to enforce maximum 1000 parameters and results.

This allows us to use u16 values internally to represents the lengths and thus we can easily use u16 instead of usize in the HostFuncEntity to inline-cache the number of parameters and results to save lots of space.

Besides that this PR has the added side benefit of improving the performance of host function calls significantly:
image

The reason is because we no longer need to RwLock to resolve a deduplicated function type with these changes.

@Robbepop Robbepop merged commit c7aa4cc into main Jul 7, 2024
18 checks passed
@Robbepop Robbepop deleted the rf-allow-instantiation-in-host-calls branch July 7, 2024 20:59
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.

1 participant