Stream: git-wasmtime

Topic: wasmtime / PR #3236 Don't parse symbol names allocating a...

Wasmtime GitHub notifications bot (Aug 24 2021 at 15:19):

alexcrichton opened PR #3236 from less-parse to main:

One of the slightly-more-expensive parts of creating a CodeMemory at
this time is how symbol names in the object are parsed to determine
whether they're for a defined function or a trampoline. Given the rigid
structure of the object file, however, we don't actually need to do
parsing and instead we can guess the SymbolIndex associated with each
desired function. This means we can eschew parsing altogether and we
never actually need to guess the name of the symbol.

This commit also defers allocation of trampoline/function maps out of
CodeMemory into the instantiation portions since that's already
rebuilding the maps and there's no need to build them twice.

<!--

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 24 2021 at 15:20):

alexcrichton edited PR #3236 from less-parse to main:

One of the slightly-more-expensive parts of creating a CodeMemory at
this time is how symbol names in the object are parsed to determine
whether they're for a defined function or a trampoline. Given the rigid
structure of the object file, however, we don't actually need to do
parsing and instead we can guess the SymbolIndex associated with each
desired function. This means we can eschew parsing altogether and we
never actually need to guess the name of the symbol.

This commit also defers allocation of trampoline/function maps out of
CodeMemory into the instantiation portions since that's already
rebuilding the maps and there's no need to build them twice.

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 24 2021 at 15:20):

alexcrichton requested cfallin for a review on PR #3236.

Wasmtime GitHub notifications bot (Aug 24 2021 at 15:27):

alexcrichton updated PR #3236 from less-parse to main.

Wasmtime GitHub notifications bot (Aug 24 2021 at 20:02):

alexcrichton updated PR #3236 from less-parse to main.

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:36):

alexcrichton updated PR #3236 from less-parse to main.

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:50):

cfallin created PR review comment:

This mapping feels pretty brittle, and likely to break accidentally with a future change. Is there a way that we could export a helper from the object crate (or factor into a third crate if that's an unwanted dep) that turns this pseudo-specified mapping into a more solid thing?

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:50):

cfallin created PR review comment:

s/is/are/ or s/trampolines/a trampoline/ (sorry for extreme grammar nitpick)

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:50):

cfallin created PR review comment:

There might be a slightly better name for this -- it seems like an escape analysis of sorts, so maybe "escaped functions"? An export is a sort of escape, and so is creation of a reference or insertion into a table.

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:50):

cfallin created PR review comment:

(On reading further down, I do see the debug-assert check of this invariant and the related one for trampoline functions, but it still feels somewhat fragile to me as a sort of distributed invariant.)

Wasmtime GitHub notifications bot (Aug 25 2021 at 15:55):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2021 at 03:52):

alexcrichton closed without merge PR #3236.

Last updated: Nov 22 2024 at 17:03 UTC