feat: add debug function index lookup - #14128
Open
smarcd wants to merge 12 commits into
Open
Conversation
* Add an alias region for data segments (bytecodealliance#13811) This also bumped the AliasRegionKey's kind bits up to 6, which triggered a mass renumbering in the disas tests. * Debug assert that all memory instructions have alias regions (bytecodealliance#13825) We should never emit a load/store instruction that isn't tagged with an alias region. * Fix alias region used when initializing passive segments (bytecodealliance#13823) This commit fixes a minor mistake to use the right alias region when initializing a passive element segment during startup. This requires using a slightly different helper than was previously done for GC references because the element segment lives in a different region than the default GC heap. --------- Co-authored-by: Alex Crichton <alex@alexcrichton.com>
* Defer DRC drops in host-side `write_gc_ref` (bytecodealliance#13842) * Defer DRC drops in host-side `write_gc_ref` This commit is an update to the DRC collector to avoid immediately dropping GC references in `write_gc_ref`. This is done today by threading contextual information such as `ExternRefHostDataTable` all the way down to `write_gc_ref` and `drop_gc_ref` hooks, but a refactoring that I'm planning to do is going to make it significantly more complicated to thread all necessary contextual information through to these hooks. Specifically I'm hoping to store `TraceInfo` outside of stores and instead inside of `Module` and `RegisteredType` to avoid the need for per-type work done during module instantiation. Threading this context through these hooks is effectively ergonomically a no-go. The strategy then taken in this commit is to change the DRC allocator, the only allocator we have that needs this information during these barriers. The DRC allocator now defers full deallocation of GC allocations to a later point in time where contextual information is available (e.g. during a GC itself). This means that host-initiated writes/drops are no longer guaranteed to actually run destructors immediately (same as with the copying collector). Internally the DRC heap already has a stack of references to decrement, and previously it was only needed during a decrement operation and now it's instead modified to persist between GC barriers through to a GC itself. * Restructure to always use the `dec_ref_stack` This updates the processing of on-stack roots to additionally get moved onto the `dec_ref_stack` in addition to host-initiated overwrites that reach a refcount of 0. * fix: use portable WASMTIME_ALIGNOF in val.h static assertions (bytecodealliance#13837) The alignment static_assert checks in val.h used __alignof (GCC extension) which returns preferred alignment rather than ABI alignment. On i686 SysV ABI these differ for uint64_t (8 vs 4), causing spurious compile failures. Replace with a WASMTIME_ALIGNOF macro that selects the correct operator per compiler: alignof (C11/C++11) for GCC/Clang and C++, __alignof for MSVC C mode which does not support alignof. The macro is #undef'd immediately after use. * Add a tunable knob for the GC heap initial size (bytecodealliance#13841) * Add Config::gc_heap_initial_size and -O gc-heap-initial-size Setting an initial heap size > 0 avoids frequent collect-then-grow cycles during instance startup. In testing with a mid-sized Kotlin component, this improves `wasmtime serve` throughput from 240 RPS to 1550 RPS when processing a single request at a time, and from 168 to 1809 RPS for concurrency == 20. * Move initial GC heap size to a tunable Also add a small smoke test plus fuzzing integration. * Drop copying collector note * Add a test for init > reservation * Fix fuzz tests --------- Co-authored-by: Till Schneidereit <till@tillschneidereit.net> * Fix missing rounding in gc initial size when fuzzing (bytecodealliance#13855) This fixes and oversight from bytecodealliance#13841 where the clamping done there during fuzzing was insufficient because the clamping happened pre-rounding which didn't match what Wasmtime did internally. * Remove `ensure_trace_info` methods (bytecodealliance#13843) * Isolate more GC-specific data to `store/gc.rs` This is an attempt to move more information that's only needed for GC-enabled Wasmtime to `gc`-feature-gated files. This moves some methods, types, etc, from `store.rs` to `store/gc.rs` * Store `TraceInfo` in registered type information This commit updates the `RegisteredType` and `TypeCollection` abstractions to inherently store within them `TraceInfo` used for GC types. This was previously calculated per-instantiation and per-store when modules were instantiated or types were inserted into the store. No consumers of this information currently exist, but the goal of this commit is to enable the next commit to use it. * Remove `ensure_trace_info` methods This commit removes all `ensure_trace_info` methods from all GC collectors, the GC store, etc. The goal of this commit is to accelerate instantiation of modules that use GC by avoiding using the read-write lock on the `TypeRegistry` stored within the engine. As shown in bytecodealliance#13822 even in read-only situations this comes with a significant performance penalty. The strategy taken in this commit is to take an alternative route of handling trace information, empowered by the previous commit. Notably trace information is now all available at `Module`-creation time, for example, and need not be re-calculated for each store. The main difficulty is then looking up this trace information at runtime when a GC is performed. This commit implements functionality where `TraceInfos` is repurposed as a cache rather than a storage table. The cache stores where the trace information is located, and then trace information is looked up where it lies at-rest within a `Module` or `RegisteredType`. This means that the first time a type is traced within a store it requires a search to determine where the trace information is located. Right now this involves two locations: * If a store's `gc_host_alloc_types` maps contains the type index, then that's where the trace information is located. * Otherwise a module previously inserted into a store's `ModuleRegistry` must have trace information. The entire registry is searched and each module is consulted to determine if it has trace information for the type index in question. The `TraceInfos` cache is intended to amortize this cost of a lookup. This lookup is additionally mitigated in the copying collector where this is only required for "big structs" where their tracing information can't be stored inline in the object header itself. Overall it's expected that for the copying collector this change has little effect on typical GC performance itself. Additionally overall, however, this eliminates usage of the read/write lock in the `TypeRegistry` entirely during instantiation. Eliminating this lock acquisition was the goal of this commit, and this is expected to help improve parallel instantiation performance of GC-using modules. * Shuffle things around to resolve compile warnings prtest:full * Fix feature gates * Run apt-get installs in a loop * More feature fixes * Cache GC subtype checks per-store (bytecodealliance#13860) * Cache GC subtype checks per-store Avoids taking a lock on the engine's type registry. Doesn't really affect single-threaded performance, but for a mid-sized Kotlin component I see performance improve from 4050 RPS to 17680 RPS. * Review comments --------- Co-authored-by: Till Schneidereit <till@tillschneidereit.net> --------- Co-authored-by: crowforkotlin <crowforkotlin@gmail.com> Co-authored-by: Till Schneidereit <till@tillschneidereit.net>
* Fix precedence in `AliasRegionKey` module bits assert (bytecodealliance#13832) The `debug_assert` parsed as `module & (!MODULE_MASK >> MODULE_OFFSET)` rather than the intended `module & !(MODULE_MASK >> MODULE_OFFSET)`. * winch(x64): Avoid undefined behavior on ctz/clz (bytecodealliance#13829) Fixes bytecodealliance#13746 Optimizes the fallback sequence emitted in clz/ctz for the x64 backend, emitting a conditional move to ensure that the destination register is always defined when the source register is zero. --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> Co-authored-by: Saúl Cabrera <saulecabrera@gmail.com>
[automatically-tag-and-release-this-commit] Co-authored-by: Wasmtime Publish <wasmtime-publish@users.noreply.github.com>
…13906) * Exclude large directories from publishing (bytecodealliance#13902) Turns out the `wasmtime-cli` `*.crate` file has ballooned to 250M+ and it's subsequently, and rightfully, failing to publish on crates.io. Exclude a whole bunch of directories not needed for building to bring the size down by quite a bit. * Update release notes
[automatically-tag-and-release-this-commit] Co-authored-by: Wasmtime Publish <wasmtime-publish@users.noreply.github.com>
…13924) * Fix call hooks with yields and concurrent execution (bytecodealliance#13871) This commit fixes two (known) issues with call hooks. The first is a long-standing issue where when Wasmtime yielded due to fuel or epochs the call hooks were not invoked. If hosts were using call hooks to track time executed in a guest, for example, then that would lead to inaccuracies. The second is with component-model-async fiber switches weren't correctly accounted for in terms of the call hook machinery which could lead results to get mixed up. The fix for both here is the same which is to sink the invocation of call hooks far down into the stack such that they cannot be forgotten when the guest exits to the host. This moves manual invocations, for example, on host functions to instead one central location that handles both host functions and libcalls. Closes bytecodealliance#13870 * Fix async-delivered write-closed events for futures (bytecodealliance#13914) An async-delivered event caused `future.drop-writable` to raise a trap when it shouldn't because the trap notification was delivered. * Release notes
[automatically-tag-and-release-this-commit] Co-authored-by: Wasmtime Publish <wasmtime-publish@users.noreply.github.com>
…14043) * Fix some API unsoundness with invalid cross-`Engine` usage (cherry picked from commit 4afa5455ad9227ad272a1f7ae9daab94a4eb601a) * Remove preemption points in bulk operations This commit updates the translation of bulk operations such as `memory.grow` which were recently refactored to not have preemption points within the operation itself. Preemption points within the operation, while useful for very large operations, expose internal and intermediate state to embedders and the rest of the runtime. For example tables that are grown are initially filled with null, which may not be valid for the table's type. These bulk operations didn't recompute pointers/indices after a possible preemption meaning if memories were grown/moved then it would cause faults. In general this is seen as too risky of an operation to perform. The fix in this commit is to move all preemption checks to the start of the operation itself. This means that bulk operations continue to be metered with a cost proportional to the size of the operation for fuel, and they all contain an initial epoch check for epochs. Once the operation is committed to, however, there's no cancelling it and it'll continue to run. In practice this means that extremely large copies, for example, can blow the epoch budget. To re-add preemption checks within the operation, however, will require very careful reintroduction to avoid these sorts of problems/faults. * Add release notes * Ignore a test on miri * Temporarily disable gating on wasi-nn testing (bytecodealliance#14000) While bytecodealliance#13892 is in the works this disables gating on wasi-nn test results. This helps resolve what looks to be ~5-10 spurious failures per day. --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
* Release Wasmtime 47.0.3 [automatically-tag-and-release-this-commit] * Attempt to fix flaky CI tests on Windows (bytecodealliance#14007) Not entirely sure if this will work, but we'll see... --------- Co-authored-by: Wasmtime Publish <wasmtime-publish@users.noreply.github.com> Co-authored-by: Alex Crichton <alex@alexcrichton.com>
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:meta", "cranelift:module", "isle", "wasmtime:c-api", "wasmtime:docs"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Contributor
|
It looks like a bunch of commits from the 47 release branch got included in your PR branch for some reason - can you please rebase this cleanly on main? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a host-only inverse to Instance::debug_function for debugging tools that need to serialize a same-instance funcref as a Wasm function index. The lookup never exposes VM pointers and returns None when guest debugging is disabled or the function is not part of the instance.\n\nTests cover private functions, imports, an unrelated host function, and disabled guest debugging.