search

oschwald / maxminddb-golang

MaxMind DB Reader for Go
ISC License
597 stars 101 forks source link

Simpeler float decoding #8

Closed alicebob closed 9 years ago

alicebob commented 9 years ago

Save allocs in the float decoding.

The decodeInt passes the tests, but I'm not familiar with the exact maxminddb encodings here.

oschwald commented 9 years ago

I don't see any speedup from this, but I'll merge it once https://github.com/maxmind/MaxMind-DB/issues/12 is clarified.

alicebob commented 9 years ago

I can split up the float and the int decoding changes in different pull requests, if that helps. The float decoder does the same as encoding/binary/Read, but without the buffers in between: http://golang.org/src/encoding/binary/binary.go#L504

Just the float change gives me a nice speedup, around 20%.

oschwald commented 9 years ago

What test are you using for the the float speed up? A city record only has two floats (latitude/longitude), so I imagine that is why I am not seeing a noticeable speedup. There would likely be a greater speedup if only the floats were being read out. Similarly, there are very few 32-bit signed integers.

Looking more closely at the integer change, I think it is functionally equivalent to the current code. The spec does need clarification, but I'll merge this as is.

alicebob commented 9 years ago

thanks!

the basic benchmark I used: https://gist.github.com/alicebob/abd21cb070041837234e (I like how the city name can be decoded to a struct, saves creating a map)

Gives me:

harmen@pixel:~/rtm/geo/s$ go test -bench=. -benchmem -cpu=1,2,4 
testing: warning: no tests to run
PASS
BenchmarkSlurp    500000          2888 ns/op          88 B/op          2 allocs/op
BenchmarkSlurp-2      500000          2894 ns/op          88 B/op          2 allocs/op
BenchmarkSlurp-4      500000          2852 ns/op          88 B/op          2 allocs/op
ok      _/home/harmen/rtm/geo/s 4.739s

I think there are too many nonsense IPs in the test. Ohwell, it still proves the patch saves allocs.