Wasmtime: refactor the pooling allocator for components #6835
Conversation
This file contains 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
fitzgen
commented
Aug 10, 2023
| /// The `MemoryAllocationIndex` was given from our `InstanceAllocator` and | ||
| /// must be given back to the instance allocator when deallocating each | ||
| /// memory. | ||
| memories: PrimaryMap<DefinedMemoryIndex, (MemoryAllocationIndex, Memory)>, |
There was a problem hiding this comment.
Unsure whether this is better as-written, or if moving the MemoryAllocationIndex into wasmtime_runtime::Memory is better. Feel free to bike shed.
github-actions
bot
added
wasmtime:api
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:config"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Label Messager: wasmtime:configIt looks like you are changing Wasmtime's configuration options. Make sure to
To modify this label's message, edit the To add new label messages or remove existing label messages, edit the |
fitzgen
force-pushed
the
refactor-pooling-allocator
branch
from
69f4a12
to
7a3070e
Compare
github-actions
bot
added
fuzzing
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "fuzzing", "wasmtime:docs"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen
force-pushed
the
refactor-pooling-allocator
branch
2 times, most recently
from
a9587b8
to
fcc174e
Compare
|
Alex is out of office till the end of the week; do you think you could take a look at this @cfallin? |
I can possibly take a look, but I'm dealing with pretty bad wrist RSI right now and trying to learn to use my machine with voice dictation so it might take quite a lot of time. if it can wait until Alex is back maybe that's better... |
|
The first two commits in this PR are tiny enough that I've just reviewed them and would be happy to sign off on them. Unfortunately the third PR is the interesting part and is a little more overwhelming, and I can't say much about it yet. On a brief skim I can at least say that moving The other thing that jumps out at me is that extracting |
|
I'd be happy to do a live review over Zoom if that would help... I'm just awfully slow at typing right now! |
fitzgen
commented
Aug 15, 2023
fitzgen
commented
Aug 15, 2023
crates/runtime/src/instance/allocator/pooling/index_allocator.rs
Outdated
Show resolved
Hide resolved
cfallin
approved these changes
Aug 15, 2023
|
FWIW, 25 instantiation benchmarks "improved", while 9 "regressed". I think this is basically all within the noise. |
We will have multiple kinds of index allocators soon, so clarify which one this is.
This will be used in future commits refactoring the pooling allocator.
We used to have one index allocator, an index per instance, and give out N tables and M memories to every instance regardless how many tables and memories they need. Now we have an index allocator for memories and another for tables. An instance isn't associated with a single instance, each of its memories and tables have an index. We allocate exactly as many tables and memories as the instance actually needs. Ultimately, this gives us better component support, where a component instance might have varying numbers of internal tables and memories. Additionally, you can now limit the number of tables, memories, and core instances a single component can allocate from the pooling allocator, even if there is the capacity for that many available. This is to give embedders tools to limit individual component instances and prevent them from hogging too much of the pooling allocator's resources.
fitzgen
force-pushed
the
refactor-pooling-allocator
branch
from
240c4b0
to
98fcc39
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
The exact `cfg`s that unlock the tests that use these are platform and feature dependent and ends up being like 5 things and super long. Simpler to just allow unused for when we are testing on other platforms or don't have the compile time features enabled.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Also fix a couple scenarios where we could leak indices if allocating an index for a memory/table succeeded but then creating the memory/table itself failed.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
eduardomourar
pushed a commit
to eduardomourar/wasmtime
that referenced
this pull request
Aug 19, 2023
…ance#6835) * Wasmtime: Rename `IndexAllocator` to `ModuleAffinityIndexAllocator` We will have multiple kinds of index allocators soon, so clarify which one this is. * Wasmtime: Introduce a simple index allocator This will be used in future commits refactoring the pooling allocator. * Wasmtime: refactor the pooling allocator for components We used to have one index allocator, an index per instance, and give out N tables and M memories to every instance regardless how many tables and memories they need. Now we have an index allocator for memories and another for tables. An instance isn't associated with a single instance, each of its memories and tables have an index. We allocate exactly as many tables and memories as the instance actually needs. Ultimately, this gives us better component support, where a component instance might have varying numbers of internal tables and memories. Additionally, you can now limit the number of tables, memories, and core instances a single component can allocate from the pooling allocator, even if there is the capacity for that many available. This is to give embedders tools to limit individual component instances and prevent them from hogging too much of the pooling allocator's resources. * Remove unused file Messed up from rebasing, this code is actually just inline in the index allocator module. * Address review feedback * Fix benchmarks build * Fix ignoring test under miri The `async_functions` module is not even compiled-but-ignored with miri, it is completely `cfg`ed off. Therefore we ahve to do the same with this test that imports stuff from that module. * Fix doc links * Allow testing utilities to be unused The exact `cfg`s that unlock the tests that use these are platform and feature dependent and ends up being like 5 things and super long. Simpler to just allow unused for when we are testing on other platforms or don't have the compile time features enabled. * Debug assert that the pool is empty on drop, per Alex's suggestion Also fix a couple scenarios where we could leak indices if allocating an index for a memory/table succeeded but then creating the memory/table itself failed. * Fix windows compile errors
geekbeast
pushed a commit
to geekbeast/wasmtime
that referenced
this pull request
Aug 21, 2023
…eature/kserve * 'feature/kserve' of github.com:geekbeast/wasmtime: Refactor Wasmtime CLI to support components (bytecodealliance#6836) Bump the wasm-tools family of crates (bytecodealliance#6861) Wasmtime: refactor the pooling allocator for components (bytecodealliance#6835)
| /// The `TableAllocationIndex` was given from our `InstanceAllocator` and | ||
| /// must be given back to the instance allocator when deallocating each | ||
| /// table. | ||
| tables: PrimaryMap<DefinedTableIndex, (TableAllocationIndex, Table)>, |
There was a problem hiding this comment.
I mentioned this in person as well, but would it be possible to eschew this index (and the one above) and infer the index from an address in the pooling allocator?
| // Every `InstanceAllocatorImpl` is an `InstanceAllocator` when used | ||
| // correctly. Also, no one is allowed to override this trait's methods, they | ||
| // must use the defaults. This blanket impl provides both of those things. | ||
| impl<T: InstanceAllocatorImpl> InstanceAllocator for T {} |
There was a problem hiding this comment.
Would it be possible to keep this as one trait? Inferring why this was split into two it seems like it wants to guarantee that the default implementations of methods are used, but this is purely internal and it's already an unsafe trait, so I think that should be enough to cover the bases? (I don't think we're at risk of duplicating these default trait method implementations anywhere)
| let table = mem::take(table); | ||
| assert!(table.is_static()); | ||
| fn decrement_core_instance_count(&self) { | ||
| self.live_core_instances.fetch_sub(1, Ordering::AcqRel); |
There was a problem hiding this comment.
Mind throwing in a debug assert here that the return value is not 0? (e.g. this never goes negative)
There was a problem hiding this comment.
(and the other decrement methods too)
This was referenced Aug 21, 2023
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
Aug 31, 2023
This commit addresses some more fallout from bytecodealliance#6835 by updating some error messages and adding clauses for new conditions. Namely: * Module compilation is now allowed to fail when the module may have more memories/tables than the pooling allocator allows per-module. * The error message for the core instance limit being reached has been updated.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 31, 2023
* Fix some warnings on nightly Rust * Fix some more fuzz-test cases from pooling changes This commit addresses some more fallout from #6835 by updating some error messages and adding clauses for new conditions. Namely: * Module compilation is now allowed to fail when the module may have more memories/tables than the pooling allocator allows per-module. * The error message for the core instance limit being reached has been updated.
eduardomourar
pushed a commit
to eduardomourar/wasmtime
that referenced
this pull request
Sep 6, 2023
…6943) * Fix some warnings on nightly Rust * Fix some more fuzz-test cases from pooling changes This commit addresses some more fallout from bytecodealliance#6835 by updating some error messages and adding clauses for new conditions. Namely: * Module compilation is now allowed to fail when the module may have more memories/tables than the pooling allocator allows per-module. * The error message for the core instance limit being reached has been updated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We used to have one index allocator, an index per instance, and give out N
tables and M memories to every instance regardless how many tables and memories
they need.
Now we have an index allocator for memories and another for tables. An instance
isn't associated with a single instance, each of its memories and tables have an
index. We allocate exactly as many tables and memories as the instance actually
needs.
Ultimately, this gives us better component support, where a component instance
might have varying numbers of internal tables and memories.
Additionally, you can now limit the number of tables, memories, and core
instances a single component can allocate from the pooling allocator, even if
there is the capacity for that many available. This is to give embedders tools
to limit individual component instances and prevent them from hogging too much
of the pooling allocator's resources.
TODO before landing:
RELEASES.mdwith a heads up about the config changes and give a small guide of how to migrate existing set ups