search

spaolacci / murmur3

Native MurmurHash3 Go implementation
BSD 3-Clause "New" or "Revised" License
947 stars 127 forks source link

implement io.StringWriter #32

Open achille-roussel opened 4 years ago

achille-roussel commented 4 years ago

Hello!

This PR adds a WriteString method to the internal digest type to implement the io.StringWriter interface and avoid memory copies when hashing strings.

I added a benchmark and ran it before and after the change to demonstrate that a heap allocation is removed by providing this method:

benchmark                   old ns/op     new ns/op     delta
BenchmarkWriteString/1      46.0          30.8          -33.04%
BenchmarkWriteString/2      46.1          31.8          -31.02%
BenchmarkWriteString/4      43.5          28.4          -34.71%
BenchmarkWriteString/8      42.1          28.9          -31.35%
BenchmarkWriteString/16     48.3          31.3          -35.20%
BenchmarkWriteString/32     54.2          34.9          -35.61%

benchmark                   old MB/s     new MB/s     speedup
BenchmarkWriteString/1      21.75        32.46        1.49x
BenchmarkWriteString/2      43.43        62.89        1.45x
BenchmarkWriteString/4      92.01        140.65       1.53x
BenchmarkWriteString/8      190.10       277.23       1.46x
BenchmarkWriteString/16     331.10       510.92       1.54x
BenchmarkWriteString/32     590.65       916.96       1.55x

benchmark                   old allocs     new allocs     delta
BenchmarkWriteString/1      1              0              -100.00%
BenchmarkWriteString/2      1              0              -100.00%
BenchmarkWriteString/4      1              0              -100.00%
BenchmarkWriteString/8      1              0              -100.00%
BenchmarkWriteString/16     1              0              -100.00%
BenchmarkWriteString/32     1              0              -100.00%

benchmark                   old bytes     new bytes     delta
BenchmarkWriteString/1      8             0             -100.00%
BenchmarkWriteString/2      8             0             -100.00%
BenchmarkWriteString/4      8             0             -100.00%
BenchmarkWriteString/8      8             0             -100.00%
BenchmarkWriteString/16     16            0             -100.00%
BenchmarkWriteString/32     32            0             -100.00%

The use of the unsafe package is a bit unfortunate, but it seemed like the package already depended on it, so this change doesn't introduce a new dependency on unsafe.

Let me know if you'd like to see anything changed!

achille-roussel commented 3 years ago

Hello @twmb, would you like to get this PR merged or would you prefer not to bring it in?

twmb commented 3 years ago

I'm not a gate on this repo, I just saw the PR and decided to look since I fixed roughly the same thing in my own fork from way back.

achille-roussel commented 3 years ago

We're maintaining a fork as well which includes this optimization https://github.com/segmentio/murmur3 (in case it is useful to someone).