Skip to content

Commit 7f3789b

Browse files
authored
Auto merge of #275 - YoniFeng:fix_assert, r=jdm
fix: move debug_assert check 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 ```rust 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 ```rust 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 swc-project/swc#6980 in a [failed test](https://github.com/swc-project/swc/actions/runs/4326536698/jobs/7554083939).
2 parents 2633751 + 120ba6c commit 7f3789b

File tree

1 file changed

+3
-5
lines changed

1 file changed

+3
-5
lines changed

src/dynamic_set.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,13 +87,11 @@ impl Set {
8787
}
8888

8989
pub(crate) fn remove(&self, ptr: *mut Entry) {
90-
let bucket_index = {
91-
let value: &Entry = unsafe { &*ptr };
92-
debug_assert!(value.ref_count.load(SeqCst) == 0);
93-
(value.hash & BUCKET_MASK) as usize
94-
};
90+
let value: &Entry = unsafe { &*ptr };
91+
let bucket_index = (value.hash & BUCKET_MASK) as usize;
9592

9693
let mut linked_list = self.buckets[bucket_index].lock();
94+
debug_assert!(value.ref_count.load(SeqCst) == 0);
9795
let mut current: &mut Option<Box<Entry>> = &mut linked_list;
9896

9997
while let Some(entry_ptr) = current.as_mut() {

0 commit comments

Comments
 (0)