search

rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.46k stars 494 forks source link

Changed registry's mutex to be RWMutex #226

Closed marcusking01 closed 6 years ago

marcusking01 commented 6 years ago

For performance gains when Get and GetOrRegister are called frequently

mihasya commented 6 years ago

Sorry about the delay in replying.

Did this show up in profile? Have you considered keeping a local pointer to the metric around instead of calling a Getter each time? I'm always suspicious when the getters show up in a profile for this library. Most likely, restructuring the code will net you larger gains than even the patch. My apologies if this is something you've already investigated and did not find sufficient.

All that said, this change does seem correct. If you could add some benchmarks that illustrate the gains, as well as some test that would exercise this functionality for the race detector, I would merge it right away. It all "looks good," but as we saw in #232, nothing is quite as good as an actual test.

marcusking01 commented 6 years ago

@mihasya In 90% of the cases we do hold a local pointer to a metric and use that reference, however in some cases where we have dynamic metric names we do use the GetOrRegister which would be optimized by using a RWMutex. I'll work on getting a benchmark and test together, I actually worked with the developer on #232 so I understand the caution here.

marcusking01 commented 6 years ago

Previous Code (Mutex) BenchmarkRegistry-8 5000000 297 ns/op BenchmarkRegistryParallel-8 20000000 111 ns/op PASS ok _/Users/mking/workspace/github/go-metrics 10.468s

New Code (RWMutex) BenchmarkRegistry-8 5000000 303 ns/op BenchmarkRegistryParallel-8 30000000 50.5 ns/op PASS ok _/Users/mking/workspace/github/go-metrics 9.132s

mihasya commented 6 years ago

Ok this looks awesome! Just to keep things tidy, would you mind creating a new branch off of current master and cherry-picking your two commits onto it? That will remove the extra "merge" commit (this can be avoided by not mergin master into the branch, and instead doing a rebase - git checkout master && git pull origin master && git checkout mybranch && git rebase master, then resolving conflicts on a per-commit basis. Likely this would have rebase with no conflicts, as the PR said it was clean to merge). Just keeps the commit history clean.

marcusking01 commented 6 years ago

See #233