Skip to content
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

reset maxprobe on empty! #51595

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

oscardssmith
Copy link
Member

@oscardssmith oscardssmith commented Oct 4, 2023

As pointed out in #51594 (comment), this is necessary for the assertion added in #49447 to be valid.

Fix #51594

As pointed out in #51594 (comment), this is necessary for the assertion added in #49447 to be valid.
@oscardssmith oscardssmith added kind:bugfix This change fixes an existing bug backport 1.10 Change should be backported to the 1.10 release labels Oct 4, 2023
@yuyichao
Copy link
Contributor

yuyichao commented Oct 4, 2023

I believe this was not enough.

Replacing the empty! with

while !isempty(deps)
    pop!(deps)
end

Still triggers the issue.

@oscardssmith
Copy link
Member Author

good catch. We also should be decrementing maxprobe on pop! if the maxprobe is greater than the size of the dict.

base/dict.jl Outdated Show resolved Hide resolved
@yuyichao
Copy link
Contributor

yuyichao commented Oct 4, 2023

If I understand correctly, max probe should not be larger than the number of element after a rehash/insertion but if we are popping and haven't done a rehash maxprobe can stay as high as the max value it reached (if we are unlucky and the element that caused the maxprobe was the last one to be popped.).

@yuyichao
Copy link
Contributor

yuyichao commented Oct 4, 2023

I think we can't maintain any invariant between the maxprobe and count during popping, unless we are deleting the last element so it could be ok if we find all places the count reaches 0 and reset the maxprobe there.

OTOH, the maxprobe < sz invariant can only be broken when the key array got resized so I think just reset the maxprobe to 0 in rehash for the h.count == 0 branch is probably good enough.

@yuyichao
Copy link
Contributor

yuyichao commented Oct 4, 2023

A brief search through the code suggests that the only place we change the size of the table is rehash! and empty!, both of which failed to reset maxprobe in the empty case. Unless there's other places that resize the table that I missed, just resetting the the maxprobe at these two places should work?.

@oscardssmith
Copy link
Member Author

That looks right to me. I'll add a test. and then this should be good to merge.

@yuyichao
Copy link
Contributor

yuyichao commented Oct 4, 2023

LGTM.

@oscardssmith oscardssmith merged commit c18e485 into master Oct 5, 2023
6 checks passed
@oscardssmith oscardssmith deleted the oscardssmith-reset-maxprobe-on-dict-empty! branch October 5, 2023 12:29
KristofferC pushed a commit that referenced this pull request Oct 9, 2023
As pointed out in
#51594 (comment),
this is necessary for the assertion added in
#49447 to be valid.

Fix #51594

(cherry picked from commit c18e485)
@KristofferC KristofferC mentioned this pull request Oct 9, 2023
31 tasks
KristofferC added a commit that referenced this pull request Nov 2, 2023
Backported PRs:
- [x] #50932 <!-- types: fix hash values of Vararg -->
- [x] #50975 <!-- Use rr-safe `nopl; rdtsc` sequence -->
- [x] #50989 <!-- fix incorrect results in `expm1(::Union{Float16,
Float32})` -->
- [x] #51284 <!-- Avoid infinite loop when doing SIGTRAP in arm64-apple
-->
- [x] #51332 <!-- Add s4 field to Xoshiro -->
- [x] #51397 <!-- call Pkg precompile hook in latest world -->
- [x] #51405 <!-- Remove fallback that assigns a module to inlined
frames. -->
- [x] #51491 <!-- Throw clearer ArgumentError for strip with two string
args -->
- [x] #51531 <!-- fix `_tryonce_download_from_cache` (busybox.exe
download error) -->
- [x] #51541 <!-- Fix string index error in tab completion code -->
- [x] #51530 <!-- Don't mark nonlocal symbols as hidden -->
- [x] #51557 <!-- Fix last startup & shutdown precompiles -->
- [x] #51512 <!-- avoid limiting Type{Any} to Type -->
- [x] #51595 <!-- reset `maxprobe` on `empty!` -->
- [x] #51582 <!-- Aggressive constprop in LinearAlgebra.wrap -->
- [x] #51592 <!-- correctly track element pointer in heap snapshot -->
- [x] #51326 <!-- complete false & true more generally as vals -->
- [x] #51376 <!-- make `hash(::Xoshiro)` compatible with `==` -->
- [x] #51557 <!-- Fix last startup & shutdown precompiles -->
- [x] #51845 
- [x] #51840 
- [x] #50663 <!-- Fix Expr(:loopinfo) codegen -->
- [x] #51863 <!-- LLVM 15.0.7-9 -->

Contains multiple commits, manual intervention needed:

- [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are
now first class -->
- [ ] #51092 <!-- inference: fix bad effects for recursion -->

Non-merged PRs with backport label:
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
- [ ] #51414 <!-- improvements on GC scheduler shutdown -->
- [ ] #51366 <!-- Handle infix operators in REPL completion -->
- [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib"
check regardless of the value of `ispath(f)` -->
- [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating
functions in Base -->
- [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.10/master regression on Dict/Set
4 participants