search

rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.43k stars 493 forks source link

speed up registry Each method #242

Closed nightlyone closed 5 years ago

nightlyone commented 5 years ago

Creating a map entry with a string type as key leads to a copy of that string in Go. Iterating via Each creates a lot of these entries, especially while reporting lots of values to a remote metrics system.

Our use case is modelled with HugeRegistry and the below benchmarks show improvements there to to the removed memory allocation.

name                  old time/op    new time/op    delta
Counter-4               9.83ns ± 0%   10.03ns ± 1%   +2.03%          (p=0.016 n=4+5)
GuageFloat64-4          62.7ns ± 4%    60.6ns ± 1%   -3.38%          (p=0.008 n=5+5)
Histogram-4              117ns ± 3%     129ns ± 4%  +10.26%          (p=0.008 n=5+5)
Registry-4               760ns ± 2%     232ns ± 2%  -69.52%          (p=0.008 n=5+5)
HugeRegistry-4          2.58ms ± 3%    0.82ms ± 1%  -68.10%          (p=0.008 n=5+5)
UniformSample257-4       108ns ± 2%     123ns ± 5%  +13.10%          (p=0.008 n=5+5)
UniformSample514-4       106ns ± 2%     118ns ± 5%  +10.90%          (p=0.008 n=5+5)
UniformSample1028-4      106ns ± 1%     119ns ± 2%  +12.43%          (p=0.008 n=5+5)

name                  old alloc/op   new alloc/op   delta
Registry-4                336B ± 0%       32B ± 0%  -90.48%          (p=0.008 n=5+5)
HugeRegistry-4           598kB ± 0%     328kB ± 0%     ~             (p=0.079 n=4+5)

name                  old allocs/op  new allocs/op  delta
Registry-4                2.00 ± 0%      1.00 ± 0%  -50.00%          (p=0.008 n=5+5)
HugeRegistry-4            5.00 ± 0%      3.00 ± 0%  -40.00%          (p=0.008 n=5+5)
mihasya commented 5 years ago

Am traveling, unlikely to look soon. Always appreciate a patch with benchmarks included, making calendar reminder to look when back. Thank you!

Alternatively, @wadey @rcrowley you around? :D

On Tue, Sep 11, 2018 at 09:24 Ingo Oeser notifications@github.com wrote:

Creating a map entry with a string type as key leads to a copy of that string in Go. Iterating via Each creates a lot of these entries, especially while reporting lots of values to a remote metrics system.

Our use case is modelled with HugeRegistry and the below benchmarks show improvements there to to the removed memory allocation.

name old time/op new time/op delta Counter-4 9.83ns ± 0% 10.03ns ± 1% +2.03% (p=0.016 n=4+5) GuageFloat64-4 62.7ns ± 4% 60.6ns ± 1% -3.38% (p=0.008 n=5+5) Histogram-4 117ns ± 3% 129ns ± 4% +10.26% (p=0.008 n=5+5) Registry-4 760ns ± 2% 232ns ± 2% -69.52% (p=0.008 n=5+5) HugeRegistry-4 2.58ms ± 3% 0.82ms ± 1% -68.10% (p=0.008 n=5+5) UniformSample257-4 108ns ± 2% 123ns ± 5% +13.10% (p=0.008 n=5+5) UniformSample514-4 106ns ± 2% 118ns ± 5% +10.90% (p=0.008 n=5+5) UniformSample1028-4 106ns ± 1% 119ns ± 2% +12.43% (p=0.008 n=5+5)

name old alloc/op new alloc/op delta Registry-4 336B ± 0% 32B ± 0% -90.48% (p=0.008 n=5+5) HugeRegistry-4 598kB ± 0% 328kB ± 0% ~ (p=0.079 n=4+5)

name old allocs/op new allocs/op delta Registry-4 2.00 ± 0% 1.00 ± 0% -50.00% (p=0.008 n=5+5) HugeRegistry-4 5.00 ± 0% 3.00 ± 0% -40.00% (p=0.008 n=5+5)

You can view, comment on, or merge this pull request online at:

https://github.com/rcrowley/go-metrics/pull/242 Commit Summary

  • speed up registry Each method

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rcrowley/go-metrics/pull/242, or mute the thread https://github.com/notifications/unsubscribe-auth/AABYRFtRoxzCY9_yWvdgPflm5mgJgCRbks5uZ2VAgaJpZM4Wivhf .

-- Sent from Russia, with love. But also from my phone.

mihasya commented 5 years ago

@nightlyone hey there! Did you intend this PR as a starting point? I just took a look, and it appears that the PR deletes a whole bunch of interfaces and implementations, including PrefixedRegistry, without replacing them. Performance optimizations are always welcome, but I can't merge something that removes functionality.

nightlyone commented 5 years ago

@mihasya Sorry, that looked like some merge-foo.

So I redid the changes. Please take a look again.

nightlyone commented 5 years ago

@mihasya wanna take a look again?

Please note that this change now conflicts with pull-request #247 I can rebase my change again on short notice and redo the benchmarks, if you want to merge #247 first.

It would really be great to get those changes merged. We are still using a fork including this change at the moment in production and I don't want to keep doing that forever :-)

nightlyone commented 5 years ago

Friendly ping @wadey @rcrowley @mihasya Is there anything I can do to move that forward?

nightlyone commented 5 years ago

Happy new year @wadey @rcrowley @mihasya ! Is there anything I can do to move that forward?

mihasya commented 5 years ago

Sorry about the silence - we've been slowly discussing moving this repo onto an actual GH org, which would allow us to actually manage it sanely. Because it's under Richard's personal org, only he is able to modify integrations. We want to make sure we don't do the move rashly and break everybody. The expectation is that a moved repo will redirect automatically, but I haven't had the time to research/verify that the git tooling will work transparently.

Basically, we are the Ents in Lord of the Rings, and we've only just said good morning 🤦‍♂️

nightlyone commented 5 years ago

2 months passed now. Any updates here? @mihasya

nightlyone commented 5 years ago

And another month passed. @mihasya / @wadey / @rcrowley are you still maintaining this library or should there be a fork started which collects all the fixes/improvements so people can move on? More than half a year to merge a PR makes it a bit difficult to usefully contribute for me.

mihasya commented 5 years ago

We deserve all the ire, I'm really sorry this is dragging on so much. The problem is that it's clear the next logical thing is to move this repo to an org so that we can more easily manage access etc. I was planning to perform the move, then fix all the CI/CD hookups, then go through and merge everything. Unfortunately the move is a sufficiently large undertaking that I haven't been able to find the time to even plan it. I will get with @rcrowley in the near future to just fix the repo in place again, work through this logjam of pull requests, then figure out the move story.

temoto commented 5 years ago

@mihasya is it possible to subscribe to news about repo move event?

nightlyone commented 5 years ago

:tada: thank you so much