Stream: git-wasmtime

Topic: wasmtime / PR #8718 impl Send for JITModule

Wasmtime GitHub notifications bot (May 31 2024 at 14:02):

MolotovCherry requested wasmtime-compiler-reviewers for a review on PR #8718.

Wasmtime GitHub notifications bot (May 31 2024 at 14:02):

MolotovCherry requested elliottt for a review on PR #8718.

Wasmtime GitHub notifications bot (May 31 2024 at 14:02):

MolotovCherry opened PR #8718 from MolotovCherry:main to bytecodealliance:main:

JITModule currently is !Send, which makes it less useful. According to bjorn, it makes sense to make it Send.

I'm fairly new to this, but please let me know any issues/concerns you have and I'll do my best to address them. Thanks!

Ref: https://bytecodealliance.zulipchat.com/#narrow/stream/206238-general/topic/Cross.20thread.20JITModule/near/441536899

cc @bjorn3

Wasmtime GitHub notifications bot (May 31 2024 at 14:03):

MolotovCherry edited PR #8718:

JITModule currently is !Send, which makes it less useful. According to bjorn, it makes sense to make it Send.

I'm fairly new to this, but please let me know any issues/concerns you have with this and I'll do my best to address them. Thanks!

Ref: https://bytecodealliance.zulipchat.com/#narrow/stream/206238-general/topic/Cross.20thread.20JITModule/near/441536899

cc @bjorn3

Wasmtime GitHub notifications bot (May 31 2024 at 14:26):

MolotovCherry submitted PR review.

Wasmtime GitHub notifications bot (May 31 2024 at 14:26):

MolotovCherry created PR review comment:

Seeing that libcall_names already originally required Send + Sync in JITBuilder, I wonder if it would be better to also add Sync to this.

Wasmtime GitHub notifications bot (May 31 2024 at 14:26):

MolotovCherry edited PR review comment.

Wasmtime GitHub notifications bot (May 31 2024 at 14:29):

MolotovCherry edited PR review comment.

Wasmtime GitHub notifications bot (May 31 2024 at 14:37):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (May 31 2024 at 14:37):

bjorn3 created PR review comment:

Instead of implementing Send on JITModule, which may cause issues if we add a new non-Send field, maybe it would be possible to implement Send on each type that doesn't currently like CompiledBlob and PtrLen. For the symbols field you can for example wrap the *const u8 in a newtype.

Wasmtime GitHub notifications bot (May 31 2024 at 14:38):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (May 31 2024 at 14:38):

bjorn3 created PR review comment:

I don't think there is much point in making it Sync too as JITModule can't be Sync anyway. For libcall_names it makes sense for it to be Sync to be able to pass cranelift_module::default_libcall_names().

Wasmtime GitHub notifications bot (May 31 2024 at 15:15):

MolotovCherry updated PR #8718.

Wasmtime GitHub notifications bot (May 31 2024 at 15:18):

MolotovCherry submitted PR review.

Wasmtime GitHub notifications bot (May 31 2024 at 15:18):

MolotovCherry created PR review comment:

Should be mostly good now I think. I made a SendWrapper for fields which can be wrapped in it in JITModule, and impl'd Send on any types that didn't like CompiledBlob and PtrLen. I also had to wrap all the NonNull fields as they were also offenders

Wasmtime GitHub notifications bot (May 31 2024 at 15:19):

MolotovCherry edited PR review comment.

Wasmtime GitHub notifications bot (May 31 2024 at 15:22):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (May 31 2024 at 15:22):

bjorn3 created PR review comment:

Deriving Copy and Clone should be fine.

Wasmtime GitHub notifications bot (May 31 2024 at 15:24):

MolotovCherry updated PR #8718.

Wasmtime GitHub notifications bot (Jun 03 2024 at 19:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 03 2024 at 20:13):

alexcrichton merged PR #8718.

Last updated: Nov 22 2024 at 16:03 UTC