Stream: git-wasmtime

Topic: wasmtime / PR #9687 [draft] show how Arc<Mmap> would work

view this post on Zulip Wasmtime GitHub notifications bot (Nov 26 2024 at 20:16):

sunshowers opened PR #9687 from sunshowers:map-at-mmap-2 to bytecodealliance:main:

Draft to show how https://github.com/bytecodealliance/wasmtime/pull/9682 would work. (Will clean this up before putting up.)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 26 2024 at 20:26):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 26 2024 at 20:26):

sunshowers created PR review comment:

This was a subtle dependency that took me a bit to understand.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 27 2024 at 22:07):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 27 2024 at 22:07):

sunshowers created PR review comment:

(And this is only dangerous when alloc represents an owned memory allocation. It's fine if alloc is borrowed. This is a subtle point.)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2024 at 15:49):

alexcrichton submitted PR review:

:+1:

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2024 at 15:49):

alexcrichton created PR review comment:

Do you think it'd perhaps be better to remove this no_clear_on_drop and instead perform it in Drop for LocalMemory? (where that could .take() and explicitly do the image stuff before dropping the mmap)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:36):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:36):

sunshowers created PR review comment:

Oh that's a good idea, similar to MemoryPool. I think it's correct.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:38):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:38):

sunshowers created PR review comment:

Oh actually there's another subtle issue here, I think if slot.instantiate below fails, we should clear on drop (that's part of the point of clear-on-drop, right?) So I think it would be best to make this change in a followup since it's a behavior change, not a refactor.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:42):

sunshowers updated PR #9687.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:42):

sunshowers has marked PR #9687 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:42):

sunshowers requested wasmtime-core-reviewers for a review on PR #9687.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:42):

sunshowers requested fitzgen for a review on PR #9687.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:43):

sunshowers submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:43):

sunshowers created PR review comment:

I've expanded this comment -- it would definitely be good to move this to the destructor, but it would be a behavior change so it should be a followup. This PR itself should not contain any behavior changes.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 06 2024 at 22:44):

sunshowers updated PR #9687.

Last updated: Dec 23 2024 at 12:05 UTC