-
-
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
WIP: try ordered Dict representation #10116
Conversation
Sorry, didn't see the PR before writing that comment. This is pretty fascinating. If it's a win whenever there's pointer indirection, it's clearly a good tradeoff for Python, where that's |
Really big gains are possible if you need the ordering feature. For example with this change,
And I get:
which is 6x faster than master, and almost 10x faster than 0.3. |
This seems like a fairly big win overall – if we can figure out a clever way to make the Int case (immediate keys in general) faster, then I would say that switching to this would be well worth it. Another benefit is that it's one less data structure to have – no need for a separate ordered dict implementation. |
So, am I correct that the thing that's slow here for dicts of Ints is calling That aside, I guess the real point of the benchmark is that inserting into an Int dict is 30% slower and iterating over an Int dict is also slower, making copy, which does both, a total of 70% slower. |
Iterating over any dict, including Int dicts, is 10x faster since it becomes equivalent to iterating over two arrays. Mostly assigning is slower. Yes, we could definitely copy more efficiently. |
bd8c9ad
to
27da02c
Compare
I rarely iterate over Dicts, so I'm curious to know how other workloads fare. For example, a way I often use Dicts is as "flexible sparse matrices," i.e.,
and sometime later if I guess I should just check this branch out and benchmark it, but I won't have time for that for the next several days. |
If you send me a test case I will happily benchmark it! |
Some of the CI jobs appear to be running out of memory here |
julia> r = Dict(1=>"hey",2=>"ho")
Dict{Int64,ASCIIString} with 2 entries:
1 => "hey"
2 => "ho"
julia> r[1]
"hey"
julia> r[2]
"ho"
julia> delete!(r,1)
Dict{Int64,ASCIIString} with 1 entry:Error showing value of type Dict{Int64,ASCIIString}:
ERROR: UndefRefError: access to undefined reference
in showdict at dict.jl:81
in writemime at replutil.jl:34
in display at REPL.jl:105
in display at REPL.jl:108
in display at multimedia.jl:149
in print_response at REPL.jl:127
in print_response at REPL.jl:112
in anonymous at REPL.jl:588
julia> r
Dict{Int64,ASCIIString} with 1 entry:Error showing value of type Dict{Int64,ASCIIString}:
ERROR: UndefRefError: access to undefined reference
in showdict at dict.jl:81
in writemime at replutil.jl:34
in display at REPL.jl:105
in display at REPL.jl:108
in display at multimedia.jl:149
in print_response at REPL.jl:127
in print_response at REPL.jl:112
in anonymous at REPL.jl:588 |
Excited about this! |
Intresting work! However, in a use case, where dict elements are deleted when going "down" in a stack and then later added back in reverse order, when going back up the stack (a simple back-tracking algoritm), this causes a (worst-case) 3x slow-down, but I suppose this is an unavoidable consequence from the strict ordering semantics. (if an element are removed and then immediately readded, it must be pushed on front right?) |
Thanks for the test case, that's very helpful! |
Ok, I was able to get back most of the performance by removing the |
Nice! Still somewhat slower, but I guess this is an extreme case of (structural) mutation relative to access.... |
Sorry for being slow, but perhaps this will be useful: function initialize(N, nnbrs)
nbrs_list = [Set{Int}() for i = 1:N]
dp = Dict{Pair{Int,Int},Float64}()
for i = 1:N
nnbr = rand(nnbrs)
nbrs = nbrs_list[i]
for j = 1:nnbr
nbr = rand(1:N)
push!(nbrs, nbr)
dp[cachekey(i,j)] = rand()
end
end
nbrs_list, dp
end
function eliminate!(nbrs_list, dp)
N = length(nbrs_list)
order = randperm(N)
for i in order
nbrs = nbrs_list[i]
for j in nbrs
delete!(dp, cachekey(i,j))
end
empty!(nbrs)
end
end
cachekey(i, j) = i <= j ? Pair(i, j) : Pair(j, i)
nnbrs = 1:20
nbrs_list, dp = initialize(5, nnbrs)
eliminate!(nbrs_list, dp)
@time 1
N = 10^5
@time nbrs_list, dp = initialize(N, nnbrs)
@time eliminate!(nbrs_list, dp) |
Is it possible this could make 0.4? As this is awesome. Does the int-case have to be special-cased for performance (e.g. using the old code)? Note: intset is special-cased (see #10065). |
If this doesn't make 0.4, it might be worth replacing OrderedDict in On Friday, May 22, 2015, Andy Hayden notifications@github.com wrote:
|
I'm starting to think we should go with this. Int keys are arguably not the most important case, and they're easy to special-case if necessary. Being faster for strings, faster iteration, and not needing a separate OrderedDict seem to be worth it. |
If you try that benchmark I posted, I'd be curious to know how it fares. |
192f8a9
to
ecf5033
Compare
The timings are a bit variable, but I got this on master:
and on this branch:
|
Best time I've seen on master:
best time here:
I think it's at least safe to conclude there's no major regression. |
LGTM! |
LGTM too... (and I do plan on using ordered Dicts heavily, so getting this in will be very nice) |
I'm all for making this change. Ordered Dicts are very useful. One issues this raises is that dicts are now distinguished not just by their contents but by their ordering. Should two Dicts be considered equal if they have different orders? While making Dicts ordered is non-breaking, changing their equality is not. |
Maybe worth merging soon? Seems like the discussion has died down and most people are pro. |
FWIW, this implementation is the one used for OrderedDicts in DataStructures.jl. |
Bump. |
I agree – we should probably do this and make let various Dict APIs take advantage of it. |
FWIW I agree with Stefan's agreement to the last "bump". |
* JuliaLang/julia#10116 * This is largely copy and paste + rename, fixups * Offers good speed improvements in OrderedDict iteration, small improvements elsewhere
* JuliaLang/julia#10116 * This is largely copy and paste + rename, fixups * Offers good speed improvements in OrderedDict iteration, small improvements elsewhere
looks like this is bit rotten, what are people's thought after 4 years? knowing we have it in |
IMO, this is a good change but we probably need to remake the branch. |
Note that this implementation was ported to |
I think this is probably a good idea. |
is this closed because it's implemented somewhere? AFAIC this is still not done and could/should be done |
This is closed due to bit-rot. It still should be done, but it needs a new PR. |
Can't we just use it from DataStructures? |
We can copy that implementation, but we still need a new PR to copy it to |
Tread cautiously about assuming the one in DataStructures is up to snuff, as it was split from Base years ago and I doubt it has seen a lot of development since. JuliaCollections/DataStructures.jl#234 in particular requires investigation. |
This implements the representation mentioned in #10092, storing keys and values in dense ordered arrays, with a sparse(r) index vector. There are a couple TODOs in the code. This is expected to be a bit slower for deletion-heavy workloads, but I haven't benchmarked that yet.
My benchmark code is at https://gist.github.com/JeffBezanson/ea224aa45e2242ca3ee9 .
I took some benchmarks from past Dict performance issues, and added a couple more. The performance characteristics are interesting. Highlights:
start
squeeze out deleted items first.rehash!
doesn't need to reallocate the keys and values arrays at all in theory, but we might need to in order to fix the problem of finalizers that remove keys during rehashing.Profiling indicates that a huge amount of insertion time is spent on the line
My best guess is that this is because
keys[si]
is random access, where before we iterated through the slots and keys arrays in order. But if pointer indirection is involved anyway (e.g. String keys) we get a significant net speedup.