Wasmtime GitHub notifications bot (Oct 04 2022 at 23:27): Wasmtime GitHub notifications bot (Oct 04 2022 at 23:27):jameysharp requested cfallin for a review on PR #5014.
Wasmtime GitHub notifications bot (Oct 04 2022 at 23:27):jameysharp requested elliottt for a review on PR #5014.
Wasmtime GitHub notifications bot (Oct 04 2022 at 23:27):jameysharp opened PR #5014 from cleanups to main:
Here's an assortment of unrelated cleanups I put together while trying to understand wasmtime-cranelift.
- 0e6333ec4 Replace resize+copy_from_slice with extend_from_slice
- f4f2d07e2 Move helpers from Context to CompiledCode
- 36a55c439 Remove some incremental-cache code duplication
- dfdff805e Remove an unnecessary #[cfg_attr]
- 7b167e776 Fix a few comments
- b76e9af86 wasmtime-cranelift: Misc cleanups
Wasmtime GitHub notifications bot (Oct 05 2022 at 00:13):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 05 2022 at 00:13):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 05 2022 at 00:13):cfallin created PR review comment:
I'd prefer that we keep the config options for
incremental-cacheat the function/module level as they are now: as a general principle, we try not to get very fine-grained with conditional compilation directives, because it makes it harder to see exactly what is included in any given configuration ("ifdef hell"). See e.g. the CoW work where (at Alex's direction actually -- I initially did stuff more like this) we have fully separate modules for the CoW and no-CoW cases that provide the same interface. I think we can just revert the one commit in the series that makes this change.
Wasmtime GitHub notifications bot (Oct 05 2022 at 00:19):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Oct 05 2022 at 00:19):jameysharp created PR review comment:
Sure, I can do that. But the cache-disabled-at-runtime case is identical to the cache-disabled-at-compile-time case. That means if I want to change the uncached case, I have to change both places and also make sure to build with the incremental-cache feature set both ways. I figured it's better to have the common path be common.
If you still want it this way, I'll factor that code out to a separate function that's called in both implementations.
Wasmtime GitHub notifications bot (Oct 05 2022 at 16:18):jameysharp updated PR #5014 from cleanups to main.
Wasmtime GitHub notifications bot (Oct 05 2022 at 16:26):jameysharp created PR review comment:
I've removed the "Remove some incremental-cache code duplication" commit and added one to factor out a
compile_uncachedfunction to use in both configurations. I hope that addresses your concern?
Wasmtime GitHub notifications bot (Oct 05 2022 at 16:26):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Oct 05 2022 at 17:33):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 05 2022 at 17:33):cfallin created PR review comment:
Much better -- thanks!
Wasmtime GitHub notifications bot (Oct 05 2022 at 17:34):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Oct 05 2022 at 17:36):jameysharp merged PR #5014.
Last updated: Nov 22 2024 at 16:03 UTC