Stream: git-wasmtime

Topic: wasmtime / PR #2984 Reimplement how instance exports are ...

Wasmtime GitHub notifications bot (Jun 14 2021 at 19:08):

alexcrichton opened PR #2984 from fix-leak to main:

This commit internally refactors how instance exports are handled and
fixes two issues. One issue is that when we instantiate an instance we
no longer forcibly load all items from the instance immediately,
deferring insertion of each item into the store data tables to happen
later as necessary. The next issue is that repeated calls to
Caller::get_export would continuously insert items into the store data
tables. While working as intended this was undesirable because it would
continuously push onto a vector that only got deallocated once the
entire store was deallocate. Now it's routed to Instance::get_export
which doesn't have this behavior.

Closes #2916
Closes #2983

<!--

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 (Jun 14 2021 at 19:10):

alexcrichton updated PR #2984 from fix-leak to main.

Wasmtime GitHub notifications bot (Jun 14 2021 at 19:54):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 14 2021 at 19:54):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 14 2021 at 19:54):

fitzgen created PR review comment:

Doesn't take too many more lines of code to implement than this TODO comment:

enum EitherIter<T, U>
where T: Iterator,
      U: Iterator<Item = T::Item>,
{
    T(T),
    U(U),
}

impl<T, U> Iterator for EitherIter<T, Y>
where T: Iterator,
      U: Iterator<Item = T::Item>,
{
    type Item = T::Item;

    fn next(&mut self) -> Option<T::Item> {
        match self {
            Self::T(t) => t.next(),
            Self::U(u) => u.next(),
        }
    }
}

Wasmtime GitHub notifications bot (Jun 14 2021 at 19:54):

fitzgen created PR review comment:

Kind of unfortunate to add a new malloc call and a couple of atomic inc refs on the instantiation hot path. Did you happen to measure if this has any observable effect on instantiation times?

Wasmtime GitHub notifications bot (Jun 14 2021 at 21:58):

alexcrichton updated PR #2984 from fix-leak to main.

Wasmtime GitHub notifications bot (Jun 14 2021 at 21:59):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 14 2021 at 21:59):

alexcrichton created PR review comment:

This was actually already happening, although it was even worse. Previously we created an IndexMap with freshly allocated strings for all the names as well. This should perform fewer allocations (only one) as well as be a bit more optimized (it's an array, should be calloc-able).

The atomics I don't think really affect anything, they're on the order of a handful of ns when we shoot for a handful of us for instantiation.

Wasmtime GitHub notifications bot (Jun 17 2021 at 14:59):

alexcrichton requested fitzgen for a review on PR #2984.

Wasmtime GitHub notifications bot (Jun 17 2021 at 14:59):

alexcrichton requested peterhuene for a review on PR #2984.

Wasmtime GitHub notifications bot (Jun 17 2021 at 18:58):

peterhuene submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2021 at 19:27):

alexcrichton merged PR #2984.

Last updated: Nov 22 2024 at 16:03 UTC