-
-
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
Use memcmp to optimize == for bit integer arrays #16877
Conversation
@@ -128,9 +128,7 @@ isless(a::AbstractString, b::AbstractString) = cmp(a,b) < 0 | |||
cmp(a::String, b::String) = lexcmp(a.data, b.data) | |||
cmp(a::Symbol, b::Symbol) = Int(sign(ccall(:strcmp, Int32, (Cstring, Cstring), a, b))) |
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.
Shouldn't Symbol
benefit from the same optimization as String
? Looks like it could have a effect in a lot of areas.
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 function already using ccall? Or do you mean a different one?
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 was thinking about ==
for Symbol
, but actually it calls ===
so I guess that's OK already.
Wow. For, me, the following julia implementation: function iseq3(A::AbstractArray, B::AbstractArray)
if size(A) != size(B)
return false
end
if isa(A,Range) != isa(B,Range)
return false
end
failures = 0
@simd for I in eachindex(A)
@inbounds failures += A[I] != B[I]
end
return failures == 0
end is (for the arrays you were testing on) much better than our current implementation (by 3-4x), but the |
@@ -653,6 +653,11 @@ function lexcmp(a::Array{UInt8,1}, b::Array{UInt8,1}) | |||
return c < 0 ? -1 : c > 0 ? +1 : cmp(length(a),length(b)) | |||
end | |||
|
|||
# use memcmp for == on integer types | |||
=={T<:Union{Int8,UInt8,Int16,UInt16,Int32,UInt32,Int64,UInt64}}(a::Array{T,1}, b::Array{T,1}) = |
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 could replace this union by the predefined BitInteger64
, or BitInteger
if you add [U]Int128
(which you should!)
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.
Awesome! I think I will probably extend this to n-dimensional arrays too; no reason to restrict to one-dimension.
Issues addressed. This final version, on my system, has the following benchmarking attributes (benchmarks are done by randomly generating arrays, then copying them, and comparing the two):
It is a bit concerning that the time taken for the new version seems slightly superlinear, but it is so much faster that the superlinearity doesn't even seem to matter. It's possible the superlinearity is an artifact of using the minimum result for The results on |
4a36ba3
to
8820d08
Compare
I've rebased this. This performance optimization should still be valid on v0.6. |
dunno if this is covered by existing benchmarks, but @nanosoldier |
# use memcmp for == on bit integer types | ||
=={T<:BitInteger,N}(a::Array{T,N}, b::Array{T,N}) = | ||
size(a) == size(b) && | ||
ccall(:memcmp, Int32, (Ptr{T}, Ptr{T}, UInt), a, b, sizeof(T) * length(a)) == 0 |
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 find it more readable to lead with the 0 ==
as opposed to having to match parens over the entire length of the line to see where the ccall starts and ends. Also if one of these is using function
form, they're similar enough that may as well use the same form in both.
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 was just lazy rebasing, sorry. Fixed.
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.
Oops, I think this might have accidentally invalidated Nanosoldier — though maybe it will still report results of the run?
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.
It just hides the status. It's probably already started by now and should still post a comment, and if the code is functionally the same and your rebase was just a style change, shouldn't matter.
8820d08
to
2f187da
Compare
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
Bump. |
I can't confirm right now, but those regressions look like noise to me. |
After #16855, it's faster to compare two arrays for equality with
String(a) == String(b)
than witha == b
. No joke!In fact, the fastest (non-String) way to compare byte arrays for equality seems to be
lexcmp
, which usesmemcmp
behind the hood:I think it makes sense to move this optimization up one level, from strings to arrays. I'm not entirely sure what the scope should be—currently it's for one-dimensional arrays of integral types that are at most 64 bits and not
Bool
, which is a bit arbitrary. It doesn't work for floating point types because ofNaN
behaviour, and I'm a little unclear about the semantics ofBool
in Julia, but I think it might work for that also, if safe.