Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify WeakVec #6587

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Simplify WeakVec #6587

merged 2 commits into from
Nov 27, 2024

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Nov 22, 2024

Resolves #6580.

Follow-up to #6419.
Related: bevyengine/bevy#15893.
@DGriffin91 / @tychedelia could you give this PR a try?

@tychedelia
Copy link
Collaborator

Just tested in Bevy with our previously degenerate case and things look a lot better! Don't have concrete numbers but the remaining small but observable hang we saw post-fix in 0.23 is basically eliminated.

@jimblandy
Copy link
Member

GitHub won't let me make suggestions on deleted lines, but:

I think push should look like this:

    pub(crate) fn push(&mut self, value: Weak<T>) {
        let original_capacity = self.inner.capacity(); 

        self.inner.push(value);

        if self.inner.capacity() > original_capacity {
            for i in (0..self.inner.len()).rev() {
                if self.inner[i].strong_count() == 0 {
                    self.inner.swap_remove(i);
                }
            }

            // Make sure our spare capacity is not (much) more than
            // twice the number of live elements. Leaving some spare
            // capacity ensures that we won't re-scan immediately.
            self.inner.shrink_to(self.inner.len() * 2);
        }
    }

By iterating backwards, you avoid needing to do any index adjustment.

This ensures that successive scans are always separated by at least as many pushes as there are elements in the set after the earlier scan.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain my suggestions are good, but please take a look. If not, I'll approve this.

@jimblandy
Copy link
Member

The most expensive operation within a single scan will be dropping the Weak, because that's what actually frees the Arc's heap block, so it hits the heap allocator. I would expect the moves of the pointers within WeakVec are almost certainly negligible in comparison to that. So probably the difference between a retain and a swap_remove implementation is relatively unimportant.

@teoxoy
Copy link
Member Author

teoxoy commented Nov 27, 2024

That's a nice solution!

I think it's good to bound the number of moves on the number of broken weak references, since we know work will be proportional to at least that in any implementation.

And it satisfies this nicely.

By iterating backwards, you avoid needing to do any index adjustment.

It also avoids moving items from the back that haven't been checked yet (possibly having to be dropped). So it gets rid of this cliff as well:

A perf cliff of swap_remove would be if all resources have been dropped...

@teoxoy
Copy link
Member Author

teoxoy commented Nov 27, 2024

One thing I changed from your proposal is instead of reallocating the whole Vec twice, we only reallocate it once.

@teoxoy teoxoy requested a review from jimblandy November 27, 2024 14:02
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@jimblandy jimblandy merged commit 1a64329 into gfx-rs:trunk Nov 27, 2024
27 checks passed
@teoxoy teoxoy deleted the simplify-weakvec branch November 27, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WeakVec should consider using Vec::swap_remove instead of a freelist
4 participants