-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Detect multiple concurent writes to Dict + remove dead code #44778
Conversation
base/dict.jl
Outdated
@@ -237,7 +232,7 @@ end | |||
h.count = count | |||
h.ndel = 0 | |||
h.maxprobe = maxprobe | |||
@assert h.age == age0 | |||
@assert h.age == age0 "Muliple concurent writes to Dict detected!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this above the assignments, so we only do the mutation if things are not broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only a sanity check for now and thus can be moved anywhere. I've also added one aging for more safety.
This seems good. Does it improve TTFP too? This call used to cause a lot of inference latency. |
(I am fairly sure that comment is also very old, and refers to a time when we assumed that doubling the size meant eliminating hash collisions–a false assumption we deleted long ago) |
Unfortunately not, TTFP seems unchanged. |
a6f8e01
to
d18b4c2
Compare
@nanosoldier |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
test/dict.jl
Outdated
end | ||
else | ||
# Multiple threads not enabled. | ||
@test false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, if multiple threads are enabled in CI. Just to make sure, it will be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea on how to enable CI with Threads.nthreads() != 1
? Going through other tests and not finding the right way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean that the following test is disabled now in CI?
Lines 30 to 32 in 897c3cf
# basic lock check | |
if nthreads() > 1 | |
let lk = Base.Threads.SpinLock() |
The test seems fundamentally flawed since there is no guarantee you actually will run into a data race, so you might get nondeterministic test failures. |
Fair point, thanks. Although the probability would be very low for the test to fail for larger dictionaries (based on how long rehashing takes), it may still happen. So, I removed the test, and PR is ready to be merged. |
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Thanks to #38180, the removed code seems to be dead because no finalizers should be used anymore to modify dictionaries (it was dangerous). Furthermore, it may help users to detect illegal concurrent writes, since it doesn't recurse and have new error message. There should be no, or even a positive, performance effect.
Btw, this seems to be the last missing step to allow manual shrinkage at
sizehint!
julia/base/dict.jl
Lines 251 to 256 in 89a613b
Master
PR