Stream: git-wasmtime

Topic: wasmtime / PR #6349 Remove the initializer from a global'...

Wasmtime GitHub notifications bot (May 05 2023 at 17:27):

alexcrichton opened PR #6349 from alexcrichton:less-global-init to bytecodealliance:main:

This commit removes the Global::initializer field to instead only store type information about the global rather than its initialization state. Instead now the initializer is stored separately from the type of the global, and only for defined globals. This removes the need in a few locations to synthesize dummy initializers.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (May 05 2023 at 17:27):

alexcrichton requested jameysharp for a review on PR #6349.

Wasmtime GitHub notifications bot (May 05 2023 at 17:27):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6349.

Wasmtime GitHub notifications bot (May 05 2023 at 17:27):

alexcrichton requested wasmtime-core-reviewers for a review on PR #6349.

Wasmtime GitHub notifications bot (May 08 2023 at 17:18):

jameysharp submitted PR review:

This is great. There's one leftover bit I think you meant to delete and one thing maybe you can make more clear, but overall this makes perfect sense.

Wasmtime GitHub notifications bot (May 08 2023 at 17:18):

jameysharp submitted PR review:

This is great. There's one leftover bit I think you meant to delete and one thing maybe you can make more clear, but overall this makes perfect sense.

Wasmtime GitHub notifications bot (May 08 2023 at 17:18):

jameysharp created PR review comment:

I assume this should be deleted instead of commented out.

Wasmtime GitHub notifications bot (May 08 2023 at 17:18):

jameysharp created PR review comment:

The connection between indexes of global_initializers and defined_global_index seems a little subtle to me, but maybe that's just because I'm not that used to these idioms. I think defined_global_index should just be subtracting off num_imported_globals, right? Could you use Iterator::zip to join the initializers to their definitions?

Feel free to leave this as is, but if you see an easy way to make this more obviously correct that'd be nice.

Wasmtime GitHub notifications bot (May 08 2023 at 18:14):

alexcrichton updated PR #6349.

Wasmtime GitHub notifications bot (May 08 2023 at 18:14):

alexcrichton created PR review comment:

Ah yes indeed this can be simplified! I've switched it to iterate over module.global_initializers which is more direct and clearer.

Wasmtime GitHub notifications bot (May 08 2023 at 18:14):

alexcrichton has enabled auto merge for PR #6349.

Wasmtime GitHub notifications bot (May 08 2023 at 18:20):

jameysharp created PR review comment:

Nice! Looks great.

Wasmtime GitHub notifications bot (May 08 2023 at 19:14):

alexcrichton merged PR #6349.

Last updated: Oct 23 2024 at 20:03 UTC