(perf) bump "string-cache" crate for per-bucket mutex #6980
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
Rust newbie.
Looking at the issue in |
|
Waiting on the breaking change getting reverted and a new |
YoniFeng
force-pushed
the
bumpStringCache
branch
from
71b5c5a
to
70b04b0
Compare
bors-servo
added a commit
to servo/string-cache
that referenced
this pull request
Mar 5, 2023
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).
YoniFeng
changed the title
YoniFeng
force-pushed
the
bumpStringCache
branch
from
7348aeb
to
279fa89
Compare
YoniFeng
force-pushed
the
bumpStringCache
branch
from
780b63b
to
e67297d
Compare
|
@kdy1 |
|
I think the post-merge benchmark is fine, as we can revert them easily |
|
Is something missing for merge? |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
Bump the "string-cache" crate to 0.8.5, which includes this PR.
Internally, it now locks on each individual bucket within the DYNAMIC_SET struct, rather than locking on the whole data structure.
Parallel performance should improve.
BREAKING CHANGE:
Related issue (if exists):