Stream: git-wasmtime

Topic: wasmtime / PR #3239 Convert compilation artifacts to just...

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:05):

alexcrichton opened PR #3239 from artifact-only-bytes to main:

This commit strips the CompilationArtifacts type down to simply a list
of bytes. This moves all extra metadata elsewhere to live within the
list of bytes itself as bincode-encoded information.

Small affordance is made to avoid an in-process
serialize-then-deserialize round-trip for use cases like Module::new,
but otherwise this is mostly just moving some data around.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:05):

alexcrichton edited PR #3239 from artifact-only-bytes to main:

This commit strips the CompilationArtifacts type down to simply a list
of bytes. This moves all extra metadata elsewhere to live within the
list of bytes itself as bincode-encoded information.

Small affordance is made to avoid an in-process
serialize-then-deserialize round-trip for use cases like Module::new,
but otherwise this is mostly just moving some data around.

cc #3230

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:06):

alexcrichton requested peterhuene for a review on PR #3239.

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:40):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:40):

bjorn3 created PR review comment:

MADV_DONTNEED could be used. For shared (file or anonymous) mappings this will cause the pages to be discarded and repopulated with the backing storage when you try to read it again. For private anonymous mappings it will repopulate with zero-pages when you try to read it again. So if you use MADV_DONTNEED when you have a shared mapping of the ELF image from a file but don't use it when you have an private anonymous mapping for non-precompiled modules I think you should be fine. You can't use it in the later case as the module may be instantiated more than once.

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:43):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:43):

bjorn3 created PR review comment:

const ELF_WASM_DATA: &'static str = ".rodata.wasm";

This is a non-writeable section.

Wasmtime GitHub notifications bot (Aug 25 2021 at 14:43):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 26 2021 at 15:48):

alexcrichton updated PR #3239 from artifact-only-bytes to main.

Wasmtime GitHub notifications bot (Aug 26 2021 at 21:54):

alexcrichton updated PR #3239 from artifact-only-bytes to main.

Wasmtime GitHub notifications bot (Aug 27 2021 at 00:29):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2021 at 00:29):

peterhuene created PR review comment:

As far as I'm aware, this behavior of MADV_DONTNEED is Linux-specific and can't be relied upon for macOS.

That said, I don't think we need to optimize for data.drop just yet.

Wasmtime GitHub notifications bot (Aug 27 2021 at 00:30):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2021 at 00:44):

peterhuene edited PR review comment.

Wasmtime GitHub notifications bot (Aug 27 2021 at 02:16):

alexcrichton updated PR #3239 from artifact-only-bytes to main.

Wasmtime GitHub notifications bot (Aug 27 2021 at 02:17):

alexcrichton merged PR #3239.

Wasmtime GitHub notifications bot (Aug 27 2021 at 11:07):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2021 at 11:07):

bjorn3 created PR review comment:

The posix specification says the following about posix_madvise:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_madvise.html

The posix_madvise() function shall have no effect on the semantics of access to memory in the specified range, although it may affect the performance of access.

This matches how linux behaves when using MADV_DONTNEED with madvise() on shared mappings, so at least for shared mappings it should be fine, right?

Wasmtime GitHub notifications bot (Aug 27 2021 at 18:28):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Aug 27 2021 at 18:28):

peterhuene created PR review comment:

The POSIX semantics are entirely advisory; as far as I know only Linux will immediately discard the pages for MADV_DONTNEED.

I'm uncertain if it has any effect on macOS or if it lazily discards the pages on memory pressure, but my understanding (which isn't necessarily correct, I might add) is that macOS doesn't implement immediate discarding like Linux does.

I think we should definitely circle back on this should it be that the data segments being resident is a problem for users (it hasn't been thus far, at least).

Last updated: Nov 22 2024 at 16:03 UTC