fitzgen requested alexcrichton for a review on PR #5049.
fitzgen opened PR #5049 from keep-existing-trap-backtraces
to main
:
This fixes some accidentally quadratic code where we would re-capture a Wasm
stack trace (takesO(n)
time) every time we propagated a trap through a host
frame back to Wasm (can happenO(n)
times). AndO(n) * O(n) = O(n^2)
, of
course. Whoops. After this commit, it trapping with a call stack that isn
frames deep of Wasm-to-host-to-Wasm calls just captures a single backtrace and
is therefore just a properO(n)
time operation, as it is intended to be.Now we explicitly track whether we need to capture a Wasm backtrace or not when
raising a trap. This unfortunately isn't as straightforward as one might hope,
however, because of the split betweenwasmtime::Trap
and
wasmtime_runtime::Trap
. We need to decide whether or not to capture a Wasm
backtrace insidewasmtime_runtime
but in order to determine whether to do that
or not we need to reflect on theanyhow::Error
and see if it is a
wasmtime::Trap
that already has a backtrace or not. This can't be done the
straightforward way because it would introduce a cyclic dependency between the
wasmtime
andwasmtime-runtime
crates. We can't merge those twoTrap
types-- at least not without effectively merging the wholewasmtime
and
wasmtime-runtime
crates together, which would be a good idea in a perfect
world but would be a ton of ocean boiling from where we currently are --
becausewasmtime::Trap
does symbolication of stack traces which relies on
module registration information data that resides inside thewasmtime
crate
and therefore can't be moved intowasmtime-runtime
. We resolve this problem by
adding a boolean towasmtime_runtime::raise_user_trap
that controls whether we
should capture a Wasm backtrace or not, and then determine whether we need a
backtrace or not at each of that function's call sites, which are inwasmtime
and therefore can do the reflection to determine whether the user trap already
has a backtrace or not. Phew!Fixes #5037
alexcrichton submitted PR review.
alexcrichton created PR review comment:
IIRC this has a
OnceCell
initialized internally here which ignores insertion of a backtrace over a prior backtrace. Could some debug assertions and such be added around here? For example can this debug assert thatneeds_backtrace
is the same asbacktrace.is_some()
? Additionallyrecord_backtrace
coulddebug_assert!
that insertion is always successful to.I'm not sure if we can actually trip the assertions but it would probably be good to be aware of any cases if there are.
alexcrichton created PR review comment:
This snippet is shared amongst a few places below as well, so couldd a helper function be added to the
wasmtime
crate which is theraise_trap(e: Error)
of old?
alexcrichton submitted PR review.
fitzgen updated PR #5049 from keep-existing-trap-backtraces
to main
.
fitzgen merged PR #5049.
Last updated: Dec 23 2024 at 12:05 UTC