Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add optional extra-checks to Wasmi executor and make checking more consistent #1217

Merged
merged 13 commits into from
Oct 3, 2024

Conversation

Robbepop
Copy link
Member

@Robbepop Robbepop commented Oct 2, 2024

This PR

  • adds a new crate feature: extra-checks
  • improves asserts and their messages for builds with debug-assertions or extra-checks enabled.
  • removes certain Wasmi executor checks that rely on Wasmi translation invariants if debug-assertions and extra-checks are disabled.
  • benchmarks conducted locally show a maximum performance penalty for enabled checks of up to 20%. However, it depends on the exact workload.

New crate feature: extra-checks

# Enables extra checks performed during Wasmi bytecode execution.
#
# These checks are unnecessary as long as Wasmi translation works as intended.
# If Wasmi translation invariants are broken due to bugs, these checks prevent
# Wasmi execution to exhibit undefined behavior (UB) in certain cases.
#
# Expected execution overhead is upt to 20%, if enabled.
#
# - Enable if your focus is on safety.
# - Disable if your focus is on execution speed.
extra-checks = []

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 5.55556% with 34 lines in your changes missing coverage. Please review.

Project coverage is 81.27%. Comparing base (55b18f1) to head (7a094d8).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/wasmi/src/engine/executor/instrs/branch.rs 18.18% 9 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/call.rs 0.00% 5 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/copy.rs 0.00% 4 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/store.rs 0.00% 4 Missing ⚠️
crates/wasmi/src/engine/executor/instrs.rs 0.00% 2 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/memory.rs 0.00% 2 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/return_.rs 0.00% 2 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/select.rs 0.00% 2 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/table.rs 0.00% 2 Missing ⚠️
crates/wasmi/src/engine/executor/instrs/load.rs 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1217      +/-   ##
==========================================
- Coverage   81.38%   81.27%   -0.11%     
==========================================
  Files         303      303              
  Lines       24973    25006      +33     
==========================================
  Hits        20324    20324              
- Misses       4649     4682      +33     

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

@Robbepop Robbepop changed the title Optimize Wasmi executor by relying more heavily on translator guarantees Add optional extra-checks to Wasmi executor and make checking more consistent Oct 2, 2024
@Robbepop Robbepop merged commit fff26d9 into main Oct 3, 2024
16 of 19 checks passed
@Robbepop Robbepop deleted the rf-use-unreachable-unchecked branch October 3, 2024 08:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant