-
-
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
Add methods to reduce
, specialized for merge
on Dict
#26440
Conversation
Thanks. Unfortunately, AFAICT I'm concerned that this change amounts to optimizing for a situation which should not really happen in the first place. Splatting shouldn't be used when the number of arguments is large. How about implementing the special |
That's strange, my change should not affect |
Sorry, I meant |
Ok. Yes, that's not an acceptable loss. I see that I'll look into it. |
I did not find a solution that is both efficient and inferable. The generic method for
Will the commit for the PR be updated, or do I need to do something more ? Of course, it would be best if the changes from the previous commit are not visible. To be useful, the method for |
Yes, that's it. You can amend the previous commit and force-push if you want to erase it.
Actually, the method can accept any array, and if the array type is concrete (e.g. |
Yes, it should do the right thing for your example. I have been concentrating on the non-trivial cases. I just meant that it it ought to do something better than return Instead, I have been following the example of |
Makes sense. I think you can just use |
I'm questioning the value of continuing this PR. The arguments against continuing are
In short, this may be premature optimization. But, I am willing to consider arguments for proceeding with the PR. |
AFAICT the time increases more than linearly, because julia> a = [Dict(rand()=>i for i in 1:1000) for j in 1:512];
julia> for i in 1:9; @btime reduce(merge, $(a[1:2^i])); end
32.840 μs (6 allocations: 68.42 KiB)
183.019 μs (25 allocations: 681.52 KiB)
432.450 μs (53 allocations: 1.73 MiB)
1.588 ms (115 allocations: 8.90 MiB)
4.604 ms (227 allocations: 25.91 MiB)
25.088 ms (457 allocations: 127.92 MiB)
87.035 ms (905 allocations: 399.95 MiB)
256.262 ms (1807 allocations: 1.27 GiB)
874.068 ms (3605 allocations: 4.75 GiB)
# Merging in-place is much faster
julia> @btime reduce(merge!, $(copy(a[1])), a)
44.914 ms (0 allocations: 0 bytes) That said, if you don't see the need for this, there's no hurry to implement this specialized method. The issue in which I mentioned this was about vectors of vectors, which are much more common and cannot be concatenated in-place as easily as dicts. |
I see your point. In number 2 in my last post I neglected to mention that efficiency also varies with the content of the dictionaries. I have been testing mostly with many dictionaries and a small set of discrete keys and values, because that was the way the problem arose in an application. |
reduce
, specialized for merge
I changed the content and title of the PR. This PR implements methods for I tested this on several cases including the one given in #26440 (comment). |
The test suite fails. This PR introduces a method ambiguity: |
Unfortunately not. Though you could implement only the method for |
That's the price for multiple dispatch. Disallowing
Yes, for instance, if the second element is a |
That's the case for named tuples, but any package could create a custom type which is not an |
Good point. The optimized version in the current PR would cut off that opportunity. I'll work on fixing that. |
I edited the PR to check that the types of all elements in an array of type The generic |
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.
Thanks. I'm a bit worried about the number of new methods, which apart from duplicating a lot of features can also introduce ambiguities. A possible approach would be this:
- Have
reduce(m::typeof(merge), items::AbstractVector)
call_merge!(reduce(typejoin, items), items::AbstractVector)
. - Have
_merge!(::Type{<:AbstractDict}, items::AbstractVector)
callreduce(merge!, _typeddictA(items), items)
, and a fallback_merge!(::Type, items::AbstractVector)
callreduce(merge, first(items), view(items, firstindex(items)+1:end))
.
This system would allow other types to define optimized methods without introducing ambiguities.
base/abstractdict.jl
Outdated
return dto | ||
end | ||
|
||
function _merge!(dto::AbstractDict, dfrom::AbstractDict) |
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.
Isn't this just merge!
?
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.
Probably. In any case, it is never called. It should be removed.
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.
Whoops! No I see it is called. It looks calling _merge!(dto::AbstractDict, dfrom::AbstractDict)
gives the same result as merge!
. But, merge!
takes splatted arguments. It would collect the single argument dfrom
into an array and then iterate over the array. I don't recall if I benchmarked it, but prima facie, it looks less efficient. My idea was to have a single method that merges two dicts. And any code (in my PR at least) would ultimately call this.
But, it does add complexity. I would not object to removing it and simply calling merge!
. Its partly a question of practical philosophy. You could argue that this optimization is premature. The goal of the PR is to remove the gross inefficiencies.
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.
I don't understand. merge!
takes splatted arguments, but that's for multiple dictionaries, not for multiple pairs. Here the method only allows two arguments, so it's equivalent to merge!
.
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.
Neither do I ! :) I wasn't thinking of pairs. I'll state how I see it and see if you or agree or see a mistake.
- We are talking about the methods
merge!(d::AbstractDict, others::AbstractDict...)
and_merge!(dto::AbstractDict, dfrom::AbstractDict)
.
2. ForDict
sa
andb
, there is no observable difference betweenmerge!(a,b)
and_merge!(a,b)
3. For the callmerge!(a,b)
, the argumentb
appears in the body in aTuple
of one element. The routine must iterate over this tuple. In_merge!(a,b)
, there is no tuple and no corresponding iteration. So the latter may be faster than the former.
I did some benchmarks and _merge!
is indeed faster. How much faster depends of course on the characteristics of the input dicts. So you gain a bit of efficiency at the expense of writing another (short) method. One can argue for or against _merge!(a,b)
by arguing that the efficiency outweighs the code complexity, or vice-versa.
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.
A concrete example:
julia> print(d1)
Dict(7=>9,9=>5,4=>3,2=>8,3=>5,5=>6,1=>1)
julia> print(d2)
Dict(7=>6,9=>6,2=>3,3=>10,5=>9,8=>10,6=>2)
julia> @btime merge!($(copy(d1)),d2);
133.953 ns (0 allocations: 0 bytes)
julia> @btime _merge!($(copy(d1)),d2);
87.529 ns (0 allocations: 0 bytes)
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.
Not sure whether the performance difference is significant given how short these timings are, but if it makes a significant difference in your concrete use case then it's worth filing a separate issue. If the difference is real a two-argument method could be added to merge!
.
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.
Well, the ratio is significant, and I think that's more important. But, the point is moot. I just wrote a discourse post about the general question. After thinking while composing the questions, I'm quite comfortable with implementing all your suggestions. I think its the better choice. As you say, maybe further optimizations can be made in the future.
base/abstractdict.jl
Outdated
return dto | ||
end | ||
|
||
function _merge!(dto::AbstractDict, dicts::AbstractArray) |
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.
This can be simplified to _merge!(dto, dicts[1], dicts[2: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.
Yes. But, shouldn't the last argument be a view
? I did benchmark the other use of view
in this PR and the increased efficiency was not insignificant.
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.
Yeah, a view might be slightly better.
base/abstractdict.jl
Outdated
|
||
function reduce(m::typeof(merge), items::AbstractVector) | ||
isempty(items) && throw(ArgumentError("Cannot merge empty vector")) | ||
length(items) == 1 && return copy(first(items)) |
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.
reduce(merge, [x])
currently returns x
without copying, so this isn't needed: you can pass an empty array when there's a single element.
Same below.
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.
I don't understand empty array
. To agree with the current behavior, we could return first(items)
. But, you are saying something else ?
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.
I mean that reduce(merge, items)
is equivalent to first(items)
when there's only one element, so no need to add special code for that case.
EDIT: or equivalent to reduce(merge, first(items), [])
.
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.
Ok. Got it. Simply remove the line and there is no observable difference. You are arguing again for simplicity over efficiency.
base/abstractdict.jl
Outdated
length(items) == 1 && return copy(first(items)) | ||
i1 = firstindex(items) + 1 | ||
i2 = lastindex(items) | ||
return reduce(m,first(items), view(items,i1:i2)) |
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.
i2
can be replaced with 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.
Good. I wasn't sure about it.
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.
Are you sure about replacing i2
with end
?
julia> a = collect(1:10);
julia> view(a, 1:end);
ERROR: syntax: missing last argument in "1:" range expression
julia> view(a, 1:lastindex(a));
julia> ex = Expr(:call,:getindex, :a, Expr(:call,:(:),1,:end))
:(getindex(a, 1:end))
julia> eval(ex)
ERROR: UndefVarError: end not defined
Now I recall having tried end
first. Must be a good reason for this.
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.
You need to use @view
instead of view
.
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.
Thanks. That will work. Anyway the method in question has been removed.
I just rebased and pushed all the changes in your last round of comments.
Thanks for reviewing. I agree with your ideas for reducing the number of methods and ambiguity. But, In point 1., shouldn't What's the most efficient way to make the changes ? Do you want to do it ? It's easy for me to make the changes and rebase once more. |
I did not refresh and see your new comments. I'll read them now... |
You haven't removed the methods as I suggested in my review. Have you tried that? |
I thought I did remove them. Maybe, I misunderstood the comments and did make all the changes. I see now that I did forget to replace |
6b575f2
to
a499d9b
Compare
It looks like these build failures are not due to the PR. The PR is pretty minimal at this point. Can't be sure of course. |
Is there a way to run this build again ? It seems likely that the failure was not due to the PR. |
If you rebase and force push the branch, CI will run again. |
Bump. Can you rebase? |
Yes. But, I have a deadline to meet tomorrow. Thanks |
EDIT: See edit at bottom. I think the rebuild is being done properly. I'm not sure of what I'm doing. I tried the following, which did nothing in the end. But, I can see that
So, I try this:
EDIT: The following has triggered a new build and test
|
This test added in this PR test fails
I'll fix this and rebase. |
@nalimilan @StefanKarpinski Unless someone else has time for a review, looks like this PR is ready to merge. |
reduce
, specialized for merge
reduce
, specialized for merge
on Dict
test/dict.jl
Outdated
@@ -900,7 +900,7 @@ end | |||
end | |||
|
|||
@testset "Dict reduce merge" begin | |||
f = (i::Vector{<:Dict}, o) -> begin | |||
function f(i::Vector{<:Dict}, o) |
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.
Should be check_merge
?
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.
Argh, I knew editing online was risky...
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.
I agree check_merge
is better. I modeled the name f
on a similar function in the file.
Bump. |
I'ts been a while, but I think there is nothing for me to do at this point. |
Nanosoldier will only be useful if there are benchmarks for |
I'll merge this tomorrow if nobody objects, as I don't want it to miss the release again. |
@@ -691,6 +691,12 @@ end | |||
|
|||
filter!(f, d::Dict) = filter_in_one_pass!(f, d) | |||
|
|||
function reduce(::typeof(merge), items::Vector{<:Dict}) |
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.
Any reason not to allow AbstractVector{<:Dict}
?
(sorry for a last minute comment!)
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.
IIRC it triggered ambiguities (that's probably buried in a comment above). It would be useful to try to generalize, but for now I'd say it's already an improvement.
Thanks @jlapeyre! Sorry it took so long... |
This is discussed in #21672.
merge
is not efficient when called with a large number ofDict
arguments. This PR makesmerge
efficient in this case.For example
master 46dcb35
0.262732 seconds (25.95 k allocations: 120.125 MiB, 23.43% gc time)
This commit
0.000516 seconds (13 allocations: 63.641 KiB)
This does not affect other methods for
merge
that I have found. For instance, for named tuples, andOrderedDict
inDataStructures
. They do not use the code that I changed.The inefficiency is due to the performance of this kind of construction
I first reimplemented the type promotion using
reduce
which is efficient. But, I replaced this with code copied fromDataStructures
, which uses a simple loop, as this is compact and very transparent.There are similar inefficiencies in other (important) methods. I have a prepared a branch fixing these in a similar way. A more general solution, which I did not investigate, is to make the construction above efficient.