alexcrichton requested fitzgen for a review on PR #8277.
alexcrichton requested wasmtime-core-reviewers for a review on PR #8277.
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 ofwasmtime-wasi
andwasi-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 throughoutwasi-common
andwasmtime-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 aMutex<HashMap<..>>
effectively and a pretty expensive one at that. TheMutex
is required because&BorrowChecker
must both allow mutations and beSync
. TheHashMap
is used to implement precise byte-level region checking to fulfill the original design requirements of whatwiggle
was envisioned to be.Given all that, this commit guts
BorrowChecker
's implementation and functionality. The type is now effectively a glorifiedRefCell
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Should there be different handle types for mut vs shared borrows?
alexcrichton submitted PR review.
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
alexcrichton merged PR #8277.
Last updated: Dec 23 2024 at 12:05 UTC