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 toInstance::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.
-->
alexcrichton updated PR #2984 from fix-leak
to main
.
fitzgen submitted PR review.
fitzgen submitted PR review.
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(), } } }
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?
alexcrichton updated PR #2984 from fix-leak
to main
.
alexcrichton submitted PR review.
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.
alexcrichton requested fitzgen for a review on PR #2984.
alexcrichton requested peterhuene for a review on PR #2984.
peterhuene submitted PR review.
alexcrichton merged PR #2984.
Last updated: Dec 23 2024 at 13:07 UTC