gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.75k stars 935 forks source link

Simplify `WeakVec` #6587

Open teoxoy opened 3 days ago

teoxoy commented 3 days ago

Resolves https://github.com/gfx-rs/wgpu/issues/6580.

Follow-up to https://github.com/gfx-rs/wgpu/pull/6419. Related: https://github.com/bevyengine/bevy/issues/15893. @DGriffin91 / @tychedelia could you give this PR a try?

tychedelia commented 13 hours ago

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 commented 9 hours ago

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.

jimblandy commented 4 hours ago

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.