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

WIP/RFC: Converted WeakKeyDict to hash & compare with object-id #28161

Closed
wants to merge 2 commits into from

Conversation

mauro3
Copy link
Contributor

@mauro3 mauro3 commented Jul 18, 2018

This implements Stefan's suggestion to use object-id hash in the WeakKeyDict: #24941 (comment), which has currently a "Triage" label. Maybe this PR can help inform that decision. The changes were pretty minimal and no breakage occurred.

TODO if we go ahead with this:

  • run package evaluator(?)
  • rename WeakKeyDict to WeakKeyIdDict
  • deprecate
  • NEWS item

I can do this on Friday if triage is positive.

TODO if we don't go ahead with this:

x-refs: #3002, #24941, #28182

@mauro3
Copy link
Contributor Author

mauro3 commented Jul 18, 2018

There was one relevant CI failure on Travis Ubuntu 32bit: https://travis-ci.org/JuliaLang/julia/jobs/405256513, essentially:

    A = [1]
    AA = copy(A)
    wkd = WeakKeyDict(A=>1, AA=>2)
    @test length(wkd)==2 # passed
    kk = collect(keys(wkd))
    @test kk[1]==kk[2] # errors with BoundsError: attempt to access 1-element Array{Array{Int32,1},1} at index [2]

This is a bit odd as length(wkd)==2 but length(collect(keys(wkd)))==1. Any ideas? I don't have a 32bit system at hand, so it will be a bit tricky to debug. Also, the other 32bit builds were fine.

@test_throws ArgumentError WeakKeyDict{Int,Any}(5.0=>1)

# WeakKeyDict hashes with object-id
AA = copy(A)
Copy link
Sponsor Member

@vtjnash vtjnash Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GC.@preserve A AA begin
...
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed! Quesion: why would they need to be preserved? The do not go out of scope and thus should not be GC'ed, no?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A is used subsequently so is relatively safe but AA isn't so it could be garbage collected.

@mauro3
Copy link
Contributor Author

mauro3 commented Jul 19, 2018

The == is actually needed: #24941 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants