wasmtime / PR #10512 cranelift-jit: add bump memory alloc... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10512 cranelift-jit: add bump memory alloc...

Wasmtime GitHub notifications bot (Apr 02 2025 at 13:14):

Mrmaxmeier opened PR #10512 from Mrmaxmeier:cranelift-jit-pluggable-memory-provider to bytecodealliance:main:

Hey,
this is a stab at #4000 (and #4986) to resolve an issue with x86 relocation range that comes up in multi-threaded JITs.
I've added an option to swap out the memory allocator in cranelift-jit and have added an arena-based option that works with a pre-allocated PROT_NONE region.

I've marked this as draft because I could also see updating the PR to either

just add the trait, have users provide their own option when in doubt?

update the existing allocator to allow reserving an area up-front?

Let me know if this seems reasonable. Thanks! :slightly_smiling_face:

Wasmtime GitHub notifications bot (Apr 02 2025 at 13:42):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 02 2025 at 13:42):

bjorn3 created PR review comment:

After finalization, has_space_for should unconditionally return false at least when this is an executable segment as any attempt to write to it will fail.

Wasmtime GitHub notifications bot (Apr 02 2025 at 13:42):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 02 2025 at 13:42):

bjorn3 created PR review comment:

*segment

Wasmtime GitHub notifications bot (Apr 02 2025 at 13:51):

Mrmaxmeier updated PR #10512.

Wasmtime GitHub notifications bot (Apr 03 2025 at 12:17):

Mrmaxmeier updated PR #10512.

Wasmtime GitHub notifications bot (Apr 03 2025 at 12:17):

Mrmaxmeier edited PR #10512:

Hey,
this is a stab at #4000 (and #4986) to resolve an issue with x86 relocation range that comes up in multi-threaded JITs.
I've added an option to swap out the memory allocator in cranelift-jit and have added an arena-based option that works with a pre-allocated PROT_NONE region.

I've marked this as draft because I could also see updating the PR to either

just add the trait, have users provide their own option when in doubt?

update the existing allocator to allow reserving an area up-front?

split the changes into multiple commits if helpful

Let me know if this seems reasonable. Thanks! :slightly_smiling_face:

Wasmtime GitHub notifications bot (Apr 03 2025 at 12:17):

Mrmaxmeier has marked PR #10512 as ready for review.

Wasmtime GitHub notifications bot (Apr 03 2025 at 12:17):

Mrmaxmeier requested abrown for a review on PR #10512.

Wasmtime GitHub notifications bot (Apr 03 2025 at 12:17):

Mrmaxmeier requested wasmtime-compiler-reviewers for a review on PR #10512.

Wasmtime GitHub notifications bot (Apr 04 2025 at 16:09):

abrown submitted PR review:

I think this overall makes sense to me; I didn't look too, too closely at all the arithmetic so it would be useful to have some tests around this (if there aren't any already). Plus, @bjorn3 are you a +1 on this? I believe you may be the one most interested in this. (I checked and apparently there are some other dependents of this so it isn't _just_ @bjorn3).

Wasmtime GitHub notifications bot (Apr 04 2025 at 16:09):

abrown created PR review comment:

It feels like there should be some page alignment concerns here since, e.g., it seems possible that ptr doesn't line up with a page boundary or len brings us up too short. I had the feeling we'd want to check that we're creating page-sized segments but, if that is for some reason not going to be a problem, perhaps we should explain how and why here in a comment.

Wasmtime GitHub notifications bot (Apr 04 2025 at 16:09):

abrown created PR review comment:

Nit: most comments throughout the project follow the convention of capitalizing sentences and ending them with a period.

Wasmtime GitHub notifications bot (Apr 04 2025 at 16:09):

abrown created PR review comment:

Oh, ok, I see where this is done later... perhaps a debug assert here would be a good double-check then?

Wasmtime GitHub notifications bot (Apr 07 2025 at 09:37):

Mrmaxmeier updated PR #10512.

Wasmtime GitHub notifications bot (Apr 07 2025 at 09:45):

Mrmaxmeier updated PR #10512.

Wasmtime GitHub notifications bot (Apr 07 2025 at 09:59):

Mrmaxmeier commented on PR #10512:

I think this overall makes sense to me; I didn't look too, too closely at all the arithmetic so it would be useful to have some tests around this (if there aren't any already). Plus, @bjorn3 are you a +1 on this? I believe you may be the one most interested in this. (I checked and apparently there are some other dependents of this so it isn't _just_ @bjorn3).

I've added some simple tests and have cleaned up the PR a bit.
Bjorn3's sub-1hr review is just luck of the draw. :slightly_smiling_face:

Wasmtime GitHub notifications bot (Apr 07 2025 at 18:48):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2025 at 18:48):

bjorn3 created PR review comment:

Can you explicitly list which items are exported? Also you don't need a newline between this and the next use if you either remove the crate:: from the next use or add it to this one.

Wasmtime GitHub notifications bot (Apr 07 2025 at 18:48):

bjorn3 created PR review comment:

This will now do a pipeline_flush_mut call per segment rather than once at the end. And this function can be relatively expensive when you have a cpu with many cores as it needs to send an ipi to every cpu core that is running the same process and wait for a reply back that the pipeline was successfully flushed.

Wasmtime GitHub notifications bot (Apr 07 2025 at 21:58):

Mrmaxmeier updated PR #10512.

Wasmtime GitHub notifications bot (Apr 07 2025 at 21:59):

Mrmaxmeier submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2025 at 21:59):

Mrmaxmeier created PR review comment:

Yes, seems reasonable to avoid the extra calls. I've moved the pipeline flush call back outside of this function :+1:

Wasmtime GitHub notifications bot (Apr 08 2025 at 09:00):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Apr 08 2025 at 14:34):

Mrmaxmeier updated PR #10512.

Wasmtime GitHub notifications bot (Apr 08 2025 at 14:35):

Mrmaxmeier commented on PR #10512:

Whoops, sorry. Didn't realize that only a subset of the CI jobs runs on posted PRs ^^

Wasmtime GitHub notifications bot (Apr 08 2025 at 22:04):

Mrmaxmeier updated PR #10512.

Wasmtime GitHub notifications bot (Apr 09 2025 at 00:41):

alexcrichton merged PR #10512.

Last updated: Apr 17 2025 at 17:03 UTC