search

segmentio / stats

Go package for abstracting stats collection
https://godoc.org/github.com/segmentio/stats
MIT License
208 stars 32 forks source link

Zero allocations for SortTags #68

Closed rbranson closed 7 years ago

rbranson commented 7 years ago

This was causing a large number of allocations since it needs to be called for every stats call, which can be VERY frequent. This patch also cuts the cost of SortTags in half.

Before: BenchmarkSortTags-2 5000000 337 ns/op 32 B/op 1 allocs/op

After: BenchmarkSortTags-2 10000000 198 ns/op 0 B/op 0 allocs/op

rbranson commented 7 years ago

@achille-roussel how long are you predicting these arrays of tags will be? The benchmark is on 7 tags, still twice as fast as the old routine.

rbranson commented 7 years ago

I tested it and it is equivalent up until ~20 tags, then it begins to slow down. Still think it's worth the fallback guard?

achille-roussel commented 7 years ago

I've never seen programs injecting more than a couple of tags, so I'd say if we have more than 20 tags we could fallback to sort.Sort (cause that would represent 400 loop iterations, I think this is where we started to see an impact when we were working on the KSUID package).

f2prateek commented 7 years ago

IIRC, sorting was added for prometheus. Do we still need to sort tags anymore without prometheus?

achille-roussel commented 7 years ago

@rbranson Yeah if we can save someone the pain of digging into high CPU usage 6 months from now it seems like a pretty small amount of effort on our end.

@f2prateek in v4 tags are now always sorted because the internal cache relies on the order of the tags.

abraithwaite commented 7 years ago

Performance penalty of having >20 tags should be punishment for having >20 tags. :-)

achille-roussel commented 7 years ago

😆