Stream: git-wasmtime

Topic: wasmtime / PR #5151 Reduce calls to `section_by_name` loa...

Wasmtime GitHub notifications bot (Oct 28 2022 at 19:14):

alexcrichton opened PR #5151 from less-section-by-name to main:

Data is stored in binary artifacts as an ELF object and when loading an artifact lots of calls are made to the object crate's section_by_name method which ends up doing a linear search through the list of sections for a particular name. To avoid doing this linear search every time I've replaced this with one loop over the sections of an object at the beginning when an object is loaded, or at least most of the calls with this loop.

This isn't really a pressing issue today but some upcoming work I hope to do for AOT-compiled components will be adding more sections to the artifact so it seems best to keep the number of linear searches small and avoided if possible.

<!--

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 (Oct 28 2022 at 21:14):

alexcrichton updated PR #5151 from less-section-by-name to main.

Wasmtime GitHub notifications bot (Oct 28 2022 at 21:26):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 21:26):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 21:26):

jameysharp created PR review comment:

I kinda wish you could pre-allocate the dwarf_sections for the maximum possible SectionId, since there aren't that many of them, rather than having to think about the edge cases of incremental resizing. But I don't see any easy way to get the max SectionId value.

I think this condition would read more clearly to me if flipped (if idx >= dwarf_sections.len()). But I always have to think about this kind of condition either way, so I don't feel strongly about it.

Wasmtime GitHub notifications bot (Oct 28 2022 at 21:37):

alexcrichton has enabled auto merge for PR #5151.

Wasmtime GitHub notifications bot (Oct 28 2022 at 21:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 21:38):

alexcrichton created PR review comment:

Ah yeah in terms of minor inefficiencies in loading a precompiled artifact I think this is pretty low down on the list, all the other operations I think vastly dominate the time spent doing that.

Wasmtime GitHub notifications bot (Oct 28 2022 at 22:08):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Oct 28 2022 at 22:08):

jameysharp created PR review comment:

Oh, absolutely. I wasn't worried about inefficiency of resizing the vector. This if condition was the only bit I had to stop to think about, and also the only place where you had a bug caught by CI. So it would be nice if we could pre-size the array to avoid having to think about the correctness of this if statement. But there doesn't appear to be a good way to do that, and after all CI _did_ catch the bug, so the way you have it is probably the best option.

Wasmtime GitHub notifications bot (Oct 28 2022 at 22:55):

alexcrichton merged PR #5151.

Last updated: Nov 22 2024 at 17:03 UTC