fix: move debug_assert check

YoniFeng commented 1 year ago

Fix needed after #268 The fix is only for debug_assert correctness. There's no functional effect. i.e. version 0.8.6 might cause debug tests to fail, but it isn't a (functionally) breaking change.

Explanation Moving the lock to be per-bucket changed the DYNAMIC_SET's API so that it doesn't need to be locked (i.e. DYANMIC_SET is not wrapped with a Mutex). The Atom's fn drop changed from

fn drop(&mut self) {
        if self.tag() == DYNAMIC_TAG {
            let entry = self.unsafe_data.get() as *const Entry;
            if unsafe { &*entry }.ref_count.fetch_sub(1, SeqCst) == 1 {
                drop_slow(self)
            }
        }

        // Out of line to guide inlining.
        fn drop_slow<Static>(this: &mut Atom<Static>) {
            DYNAMIC_SET
                .lock()
                .remove(this.unsafe_data.get() as *mut Entry);
        }
    }

to

impl<Static> Drop for Atom<Static> {
    #[inline]
    fn drop(&mut self) {
        if self.tag() == DYNAMIC_TAG {
            let entry = self.unsafe_data.get() as *const Entry;
            if unsafe { &*entry }.ref_count.fetch_sub(1, SeqCst) == 1 {
                drop_slow(self)
            }
        }

        // Out of line to guide inlining.
        fn drop_slow<Static>(this: &mut Atom<Static>) {
            DYNAMIC_SET.remove(this.unsafe_data.get() as *mut Entry);
        }
    }

(the lock() is gone) Now when we enter DYNAMIC_SET.remove(), there's no longer a lock guarantee. Until we lock the bucket, another thread could be in the middle of performing a DYNAMIC_SET.insert() for the same string. Therefore, the debug_assert!(value.ref_count.load(SeqCst) == 0) is premature - it needs to occur after we take the lock for the bucket.

Caught at https://github.com/swc-project/swc/pull/6980 in a failed test.