Stream: git-wasmtime

Topic: wasmtime / issue #3196 Move module translation from crane...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 17:56):

alexcrichton commented on issue #3196:

As a heads up there's a ton of non-obvious code movement in this PR. While it can be reviewed in detail the diff is quite large. It was not my intention to actually modify any behavior in this PR, this should in theory be only a large refactoring.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 17:57):

bjorn3 commented on issue #3196:

Does Spidermonkey need this function?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 17:58):

bjorn3 edited a comment on issue #3196:

Does Spidermonkey need this?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 17:59):

bjorn3 edited a comment on issue #3196:

Does Spidermonkey need the module translation code too? Or does it write its own code?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 18:15):

cfallin commented on issue #3196:

Does Spidermonkey need the module translation code too? Or does it write its own code?

SpiderMonkey only uses the per-function translation API surface, so AFAIK this should not break it. The relevant code is here; see e.g. how BatchCompiler holds a FuncTranslator but not anything at the module level.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 18:17):

cfallin edited a comment on issue #3196:

Does Spidermonkey need the module translation code too? Or does it write its own code?

SpiderMonkey only uses the per-function translation API surface, so AFAIK this should not break it. The relevant code is here; see e.g. how BatchCompiler holds a FuncTranslator but not anything at the module level (edit: or at least, not the module translator).

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 19:24):

pchickey commented on issue #3196:

Lucet also uses ModuleEnvironment, so we will need to inline the implementation over there so that we can keep on putting cranelift bugfixes into Lucet.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:14):

alexcrichton commented on issue #3196:

@pchickey would you prefer if I just left cranelift-wasm alone in this regard? I could still inline everything into wasmtime-environ and just leave the crate alone and achieve the same goals here.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:27):

pchickey commented on issue #3196:

Yeah, that should be fine for now. We can add a deprecation warning for now, and delete the dead code in cranelift-wasm in a little while after lucet is finally dead

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 21:29):

alexcrichton commented on issue #3196:

Ok I've updated to avoid changing cranelift-wasm and I've updated the commit/PR description as well.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 21:30):

alexcrichton commented on issue #3196:

One worthwhile thing to point out is that with Wasmtime no longer using translate_module I think that means that there will be no more active maintainers of that functionality in cranelift-wasm any more.

Last updated: Dec 23 2024 at 12:05 UTC