Avoid cloning and dropping Arcs in LocalMemory::vmmemory
#11148
Merged
Conversation
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
Getting the base pointer from the underlying `dyn RuntimeLinearMemory` involved getting a `MemoryBase` which is potentially an `MmapOffset` which itself contains an `Arc<Mmap>`, and we would then call `base.as_non_null()` to turn this into a raw pointer, and then we would drop the `MemoryBase` which ultimately drops the `Arc<Mmap>`. So that's an `Arc` clone and drop just to get a `VMMemoryDefinition`, which is just a pointer and a length, essentially a slice of the linear memory. And, among other places, we call `LocalMemory::vmmemory` to get the GC heap's memory base and bound every time we access a GC object from Rust (for example, during collections). Altogether, this removes another 30% of runtime from the testcase in bytecodealliance#11141
|
All that said, I don't love how this PR introduces a second version of the "same" method. It might be nicer to instead add a That would actually let us avoid an indirect call too:
Note that the "now" sequence has two indirect calls while the alternative has only one. I'm going to whip this alternative up real quick and then update this PR. |
…tation Instead of in `LocalMemory` via calling various `RuntimeLinearMemory` methods, as `LocalMemory` has a `dyn RuntimeLinearMemory` object so those calls are all indirect.
I've pushed a commit that implements this. |
alexcrichton
approved these changes
Jun 27, 2025
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.
Getting the base pointer from the underlying
dyn RuntimeLinearMemoryinvolved getting aMemoryBasewhich is potentially anMmapOffsetwhich itself contains anArc<Mmap>, and we would then callbase.as_non_null()to turn this into a raw pointer, and then we would drop theMemoryBasewhich ultimately drops theArc<Mmap>. So that's anArcclone and drop just to get aVMMemoryDefinition, which is just a pointer and a length, essentially a slice of the linear memory. And, among other places, we callLocalMemory::vmmemoryto get the GC heap's memory base and bound every time we access a GC object from Rust (for example, during collections).Altogether, this removes another 30% of runtime from the testcase in #11141