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.

Sign up for GitHub

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

Use @inbounds annotations for Dict implementation #23843

Merged
merged 2 commits into from
Oct 2, 2017
Merged

Use @inbounds annotations for Dict implementation #23843

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc33a40
Use `@inbounds` annotations for `Dict` implementation
Sep 23, 2017
7e9aca3
Use `@propagate_inbounds` instead of `@inbounds` for `next`
andyferris Sep 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 30 additions & 24 deletions base/dict.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,9 @@ convert(::Type{Dict{K,V}},d::Dict{K,V}) where {K,V} = d

hashindex(key, sz) = (((hash(key)%Int) & (sz-1)) + 1)::Int

isslotempty(h::Dict, i::Int) = h.slots[i] == 0x0
isslotfilled(h::Dict, i::Int) = h.slots[i] == 0x1
isslotmissing(h::Dict, i::Int) = h.slots[i] == 0x2
@propagate_inbounds isslotempty(h::Dict, i::Int) = h.slots[i] == 0x0
@propagate_inbounds isslotfilled(h::Dict, i::Int) = h.slots[i] == 0x1
@propagate_inbounds isslotmissing(h::Dict, i::Int) = h.slots[i] == 0x2

function rehash!(h::Dict{K,V}, newsz = length(h.keys)) where V where K
olds = h.slots
Expand All @@ -238,7 +238,7 @@ function rehash!(h::Dict{K,V}, newsz = length(h.keys)) where V where K
maxprobe = h.maxprobe

for i = 1:sz
if olds[i] == 0x1
@inbounds if olds[i] == 0x1
k = oldk[i]
v = oldv[i]
index0 = index = hashindex(k, newsz)
Expand Down Expand Up @@ -328,7 +328,7 @@ function ht_keyindex(h::Dict{K,V}, key) where V where K
index = hashindex(key, sz)
keys = h.keys

while true
@inbounds while true
if isslotempty(h,index)
break
end
Expand All @@ -355,7 +355,7 @@ function ht_keyindex2(h::Dict{K,V}, key) where V where K
avail = 0
keys = h.keys

while true
@inbounds while true
if isslotempty(h,index)
if avail < 0
return avail
Expand All @@ -382,7 +382,7 @@ function ht_keyindex2(h::Dict{K,V}, key) where V where K

maxallowed = max(maxallowedprobe, sz>>maxprobeshift)
# Check if key is not present, may need to keep searching to find slot
while iter < maxallowed
@inbounds while iter < maxallowed
if !isslotfilled(h,index)
h.maxprobe = iter
return -index
Expand All @@ -396,7 +396,7 @@ function ht_keyindex2(h::Dict{K,V}, key) where V where K
return ht_keyindex2(h, key)
end

function _setindex!(h::Dict, v, key, index)
@propagate_inbounds function _setindex!(h::Dict, v, key, index)
h.slots[index] = 0x1
h.keys[index] = key
h.vals[index] = v
Expand Down Expand Up @@ -428,10 +428,10 @@ function setindex!(h::Dict{K,V}, v0, key::K) where V where K

if index > 0
h.age += 1
h.keys[index] = key
h.vals[index] = v
@inbounds h.keys[index] = key
@inbounds h.vals[index] = v
else
_setindex!(h, v, key, -index)
@inbounds _setindex!(h, v, key, -index)
end

return h
Expand Down Expand Up @@ -501,10 +501,10 @@ function get!(default::Callable, h::Dict{K,V}, key::K) where V where K
end
if index > 0
h.age += 1
h.keys[index] = key
h.vals[index] = v
@inbounds h.keys[index] = key
@inbounds h.vals[index] = v
else
_setindex!(h, v, key, -index)
@inbounds _setindex!(h, v, key, -index)
end
return v
end
Expand All @@ -520,7 +520,7 @@ end

function getindex(h::Dict{K,V}, key) where V where K
index = ht_keyindex(h, key)
return (index < 0) ? throw(KeyError(key)) : h.vals[index]::V
@inbounds return (index < 0) ? throw(KeyError(key)) : h.vals[index]::V
end

"""
Expand All @@ -544,7 +544,7 @@ get(collection, key, default)

function get(h::Dict{K,V}, key, default) where V where K
index = ht_keyindex(h, key)
return (index < 0) ? default : h.vals[index]::V
@inbounds return (index < 0) ? default : h.vals[index]::V
end

"""
Expand All @@ -566,7 +566,7 @@ get(::Function, collection, key)

function get(default::Callable, h::Dict{K,V}, key) where V where K
index = ht_keyindex(h, key)
return (index < 0) ? default() : h.vals[index]::V
@inbounds return (index < 0) ? default() : h.vals[index]::V
end

"""
Expand Down Expand Up @@ -610,7 +610,7 @@ julia> getkey(a,'d','a')
"""
function getkey(h::Dict{K,V}, key, default) where V where K
index = ht_keyindex(h, key)
return (index<0) ? default : h.keys[index]::K
@inbounds return (index<0) ? default : h.keys[index]::K
end

function _pop!(h::Dict, index)
Expand Down Expand Up @@ -656,8 +656,8 @@ end
function pop!(h::Dict)
isempty(h) && throw(ArgumentError("dict must be non-empty"))
idx = start(h)
key = h.keys[idx]
val = h.vals[idx]
@inbounds key = h.keys[idx]
@inbounds val = h.vals[idx]
_delete!(h, idx)
key => val
end
Expand Down Expand Up @@ -701,7 +701,7 @@ end

function skip_deleted(h::Dict, i)
L = length(h.slots)
while i<=L && !isslotfilled(h,i)
@inbounds while i<=L && !isslotfilled(h,i)
i += 1
end
return i
Expand All @@ -713,13 +713,19 @@ function start(t::Dict)
return i
end
done(t::Dict, i) = i > length(t.vals)
next(t::Dict{K,V}, i) where {K,V} = (Pair{K,V}(t.keys[i],t.vals[i]), skip_deleted(t,i+1))
function next(t::Dict{K,V}, i) where {K,V}
@inbounds return (Pair{K,V}(t.keys[i],t.vals[i]), skip_deleted(t,i+1))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is safe --- it's possible some user code could pass a bad value of i by mistake.

Copy link
Member

Choose a reason for hiding this comment

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

These could be marked @propagate_inbounds, though; that seems to be what arrays do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ref #15291

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm assuming that what is now here makes sense? And that for loops expand/lower to include @inbounds next(...) for fast iteration over e.g. Array?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, for loop cannot be lowered to @inbounds. It only guarantee that the iterator protocol is properly executed, but not inbounds access in next.

Copy link
Member Author

@andyferris andyferris Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Umm... so are you telling me that for x in vector isn't optimally fast? It really spends time checking bounds, immediately after it has verified done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh dear, I see #15291 wasn't merged... (I figured it was merged and we've moved onto something simpler, but alas)

Copy link
Member Author

@andyferris andyferris Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @yuyichao for clearing that up. At least with @propagate_inbounds here, users have the option of @inbounds for kv in dict for a speedup, right?

Also, I'll have to update my benchmark.

end

isempty(t::Dict) = (t.count == 0)
length(t::Dict) = t.count

next(v::KeyIterator{<:Dict}, i) = (v.dict.keys[i], skip_deleted(v.dict,i+1))
next(v::ValueIterator{<:Dict}, i) = (v.dict.vals[i], skip_deleted(v.dict,i+1))
function next(v::KeyIterator{<:Dict}, i)
@inbounds return (v.dict.keys[i], skip_deleted(v.dict,i+1))
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

end
function next(v::ValueIterator{<:Dict}, i)
@inbounds return (v.dict.vals[i], skip_deleted(v.dict,i+1))
end

function filter_in_one_pass!(f, d::Associative)
try
Expand Down