Skip to content

Commit

Permalink
fix slow hash(::SubString{<:ByteString})
Browse files Browse the repository at this point in the history 
Cuts the #8826 vocab.jl benchmark in half.
  • Loading branch information
@nolta
nolta committed Nov 6, 2014
1 parent cd450f6 commit 4b4565f
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion base/string.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ const memhash_seed = UInt === UInt64 ? 0x71e729fd56419c81 : 0x56419c81

function hash{T<:ByteString}(s::Union(T,SubString{T}), h::UInt)
h += memhash_seed
ccall(memhash, UInt, (Ptr{UInt8}, Csize_t, UInt32), s, sizeof(s), h % UInt32) + h
ccall(memhash, UInt, (Ptr{UInt8}, Csize_t, UInt32), pointer(s), sizeof(s), h % UInt32) + h
end
hash(s::AbstractString, h::UInt) = hash(bytestring(s), h)

Expand Down

8 comments on commit 4b4565f

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow. We can avoid similar problems in the future by adding a definition for cconvert of SubStrings in base.jl.

@catawbasam
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@nolta
Copy link
Member Author

@nolta nolta commented on 4b4565f Nov 6, 2014

Choose a reason for hiding this comment

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

I thought about changing cconvert, but i think in most cases you'll want a NUL-terminated string.

@stevengj
Copy link
Member

Choose a reason for hiding this comment

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

I thought this was already fixed in #6058... I don't see an easy way of adding a non-benchmark regression test for this, though.

@nolta
Copy link
Member Author

@nolta nolta commented on 4b4565f Nov 7, 2014

Choose a reason for hiding this comment

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

1a670f9

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

My bad.

@ivarne
Copy link
Member

@ivarne ivarne commented on 4b4565f Nov 7, 2014

Choose a reason for hiding this comment

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

@stevengj I think what you are looking for is called a comment that explains why the code looks strange.

@stevengj
Copy link
Member

Choose a reason for hiding this comment

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

@ivarne, fair enough; added in 2747864

Please sign in to comment.