-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Performance improvements for IdDict #51091
base: master
Are you sure you want to change the base?
Changes from 14 commits
717388b
27dcc24
a31404b
d5fba6f
d17b27b
bb360d6
f377b7e
e34913a
46daa2d
cd5f495
ad5a02a
c3047c1
3a428ad
25e7410
6cbd3f0
e864462
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -24,6 +24,7 @@ IdDict{Any, String} with 3 entries: | |||||||||||||||||
``` | ||||||||||||||||||
""" | ||||||||||||||||||
mutable struct IdDict{K,V} <: AbstractDict{K,V} | ||||||||||||||||||
# NOTE make sure to sync the struct definition with `jl_id_dict_t` in julia.h | ||||||||||||||||||
ht::Memory{Any} | ||||||||||||||||||
count::Int | ||||||||||||||||||
ndel::Int | ||||||||||||||||||
|
@@ -83,18 +84,35 @@ function sizehint!(d::IdDict, newsz) | |||||||||||||||||
rehash!(d, newsz) | ||||||||||||||||||
end | ||||||||||||||||||
|
||||||||||||||||||
function setindex!(d::IdDict{K,V}, @nospecialize(val), @nospecialize(key)) where {K, V} | ||||||||||||||||||
# get (index) for the key | ||||||||||||||||||
# index - where a key is stored, or -pos if not present | ||||||||||||||||||
# and was inserted at pos | ||||||||||||||||||
function ht_keyindex2!(d::IdDict{K,V}, @nospecialize(key)) where {K, V} | ||||||||||||||||||
!isa(key, K) && throw(KeyTypeError(K, key)) | ||||||||||||||||||
if !(val isa V) # avoid a dynamic call | ||||||||||||||||||
val = convert(V, val)::V | ||||||||||||||||||
end | ||||||||||||||||||
return ccall(:jl_eqtable_keyindex, Cssize_t, (Any, Any), d, key) | ||||||||||||||||||
end | ||||||||||||||||||
|
||||||||||||||||||
@propagate_inbounds function _setindex!(d::IdDict{K,V}, val::V, keyindex::Int) where {K, V} | ||||||||||||||||||
d.ht[keyindex+1] = val | ||||||||||||||||||
d.count += 1 | ||||||||||||||||||
|
||||||||||||||||||
if d.ndel >= ((3*length(d.ht))>>2) | ||||||||||||||||||
rehash!(d, max((length(d.ht)%UInt)>>1, 32)) | ||||||||||||||||||
d.ndel = 0 | ||||||||||||||||||
end | ||||||||||||||||||
inserted = RefValue{Cint}(0) | ||||||||||||||||||
d.ht = ccall(:jl_eqtable_put, Memory{Any}, (Any, Any, Any, Ptr{Cint}), d.ht, key, val, inserted) | ||||||||||||||||||
d.count += inserted[] | ||||||||||||||||||
return nothing | ||||||||||||||||||
end | ||||||||||||||||||
|
||||||||||||||||||
function setindex!(d::IdDict{K,V}, @nospecialize(val), @nospecialize(key)) where {K, V} | ||||||||||||||||||
if !(val isa V) # avoid a dynamic call | ||||||||||||||||||
val = convert(V, val)::V | ||||||||||||||||||
end | ||||||||||||||||||
keyindex = ht_keyindex2!(d, key) | ||||||||||||||||||
if keyindex >= 0 | ||||||||||||||||||
@inbounds d[keyindex] = val | ||||||||||||||||||
else | ||||||||||||||||||
@inbounds _setindex!(d, val, -keyindex) | ||||||||||||||||||
end | ||||||||||||||||||
return d | ||||||||||||||||||
end | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -153,16 +171,24 @@ end | |||||||||||||||||
|
||||||||||||||||||
length(d::IdDict) = d.count | ||||||||||||||||||
|
||||||||||||||||||
isempty(d::IdDict) = length(d) == 0 | ||||||||||||||||||
|
||||||||||||||||||
copy(d::IdDict) = typeof(d)(d) | ||||||||||||||||||
|
||||||||||||||||||
function get!(d::IdDict{K,V}, @nospecialize(key), @nospecialize(default)) where {K, V} | ||||||||||||||||||
val = ccall(:jl_eqtable_get, Any, (Any, Any, Any), d.ht, key, secret_table_token) | ||||||||||||||||||
if val === secret_table_token | ||||||||||||||||||
keyindex = ht_keyindex2!(d, key) | ||||||||||||||||||
|
||||||||||||||||||
if keyindex < 0 | ||||||||||||||||||
# If convert call fails we need the key to be deleted | ||||||||||||||||||
@inbounds d[-keyindex] = nothing | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the deleted state not require they key to be
This comment was marked as outdated.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, the segfault was because I meant to write Lines 63 to 69 in e22db88
I can move the key updating code out of |
||||||||||||||||||
d.ndel += 1 | ||||||||||||||||||
val = isa(default, V) ? default : convert(V, default)::V | ||||||||||||||||||
setindex!(d, val, key) | ||||||||||||||||||
return val | ||||||||||||||||||
else | ||||||||||||||||||
d.ndel -= 1 | ||||||||||||||||||
@inbounds d[-keyindex] = key | ||||||||||||||||||
@inbounds _setindex!(d, val, -keyindex) | ||||||||||||||||||
return val::V | ||||||||||||||||||
else | ||||||||||||||||||
return @inbounds d.ht[keyindex+1]::V | ||||||||||||||||||
end | ||||||||||||||||||
end | ||||||||||||||||||
|
||||||||||||||||||
|
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.
There seems to be a discrepancy here in the state of the IdDict during this convert call or if this convert failed. The
ht_keyindex!
call has fully inserted the key, but not the value. ForDict
, neither has occurred yet, and all insertion and accounting happens at once in_setindex!
. But in IdDict here, it looks like half of the insertion occurs inht_keyindex!
(effectively inserting it in the deleted state) but then the final accounting update for it doesn't happen until after the_setindex!
call later in this method. Do we updatendel
over this call so the value is consistent?There also may need to be an
age
counter added to the IdDict, so it can detect concurrent modifications that happen in thisconvert
call (or move this convert call back to the top of the function)