Stream: git-wasmtime

Topic: wasmtime / PR #8277 Update wiggle's `BorrowChecker` imple...

Wasmtime GitHub notifications bot (Apr 01 2024 at 15:03):

alexcrichton requested fitzgen for a review on PR #8277.

Wasmtime GitHub notifications bot (Apr 01 2024 at 15:03):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8277.

Wasmtime GitHub notifications bot (Apr 01 2024 at 15:03):

alexcrichton opened PR #8277 from alexcrichton:less-borrow-checking to bytecodealliance:main:

This commit is a refactoring and modernization of wiggle's BorrowChecker implementation. This type is quite old and predates everything related to the component model for example. This type additionally predates the implementation of WASI threads for Wasmtime as well. In general, this type is old and has not been updated in a long time.

Originally a BorrowChecker was intended to be a somewhat cheap method of enabling the host to have active safe shared and mutable borrows to guest memory. Over time though this hasn't really panned out. The WASI threads proposal, for example, doesn't allow safe shared or mutable borrows at all. Instead everything must be modeled as a copy in or copy out of data. This means that all of wasmtime-wasi and wasi-common have largely already been rewritten in such a way to minimize borrows into linear memory.

Nowadays the only types that represent safe borrows are the GuestSlice type and its equivalents (e.g. GuestSliceMut, GuestStr, etc). These are minimally used throughout wasi-common and wasmtime-wasi and when they are used they're typically isolated to a small region of memory.

This is all coupled with the facst that BorrowChecker never ended up being optimized. It's a Mutex<HashMap<..>> effectively and a pretty expensive one at that. The Mutex is required because &BorrowChecker must both allow mutations and be Sync. The HashMap is used to implement precise byte-level region checking to fulfill the original design requirements of what wiggle was envisioned to be.

Given all that, this commit guts BorrowChecker's implementation and functionality. The type is now effectively a glorified RefCell for the entire span of linear memory. Regions are no longer considered when borrows are made and instead a shared borrow is considered as borrowing the entirety of shared memory. This means that it's not possible to simultaneously have a safe shared and mutable borrow, even if they're disjoint, at the same time.

The goal of this commit is to address performance issues seen in #7973 which I've seen locally as well. The heavyweight implementation of BorrowChecker isn't really buying us much nowadays, especially with much development having since moved on to the component model. The hope is that this much coarser way of implementing borrow checking, which should be much more easily optimizable, is sufficient for the needs of WASI and not a whole lot else.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Apr 01 2024 at 16:11):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 01 2024 at 16:11):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Apr 01 2024 at 16:11):

fitzgen created PR review comment:

Should there be different handle types for mut vs shared borrows?

Wasmtime GitHub notifications bot (Apr 01 2024 at 17:42):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 01 2024 at 17:42):

alexcrichton created PR review comment:

In theory yeah, but I'm not too keen to do too much refactoring here because it's all basically a dated library on life support now. These are all ignored anyway so I should probably refactor to remove them entirely, but I'll leave that as a future TODO if necessary

Wasmtime GitHub notifications bot (Apr 01 2024 at 18:09):

alexcrichton merged PR #8277.

Last updated: Oct 23 2024 at 20:03 UTC