Avoid expensive operations in CaptureEvent hot path #256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The new test checks that the effective sample rate (events_sent/events_seen ratio) converges to the configured ClientOptions.SampleRate. Additionally, because client values are shared among hubs and used concurrently from multiple goroutines, the test simulates the concurrent use of a client to detect data races.
The host name typically do not change for the lifetime of a program. A CPU profile from BenchmarkProcessEvent shows that most of the time is spent on syscalls to get the host name. We can precompute the host name once during initialization and avoid further syscalls, speeding up Client.prepareEvent and, transitively, Client.processEvent. name old time/op new time/op delta ProcessEvent-16 10.6µs ±12% 9.6µs ±12% -9.31% (p=0.021 n=8+8) name old alloc/op new alloc/op delta ProcessEvent-16 6.49kB ± 0% 6.45kB ± 1% -0.72% (p=0.002 n=8+8) name old allocs/op new allocs/op delta ProcessEvent-16 9.00 ± 0% 7.62 ± 8% -15.28% (p=0.000 n=8+8)
It is wasteful for the client to allocate a new random number generator for every processed event. Instead, use a single generator, shared and safe for concurrent use. name old time/op new time/op delta ProcessEvent-16 9.08µs ± 2% 0.78µs ± 1% -91.38% (p=0.000 n=8+8) name old alloc/op new alloc/op delta ProcessEvent-16 6.41kB ± 0% 1.02kB ± 0% -84.02% (p=0.000 n=8+8) name old allocs/op new allocs/op delta ProcessEvent-16 7.00 ± 0% 6.00 ± 0% -14.29% (p=0.002 n=7+8)
kamilogorek
reviewed
Jun 26, 2020
| // tolerated range is [SampleRate-MaxDelta, SampleRate+MaxDelta] | ||
| MaxDelta float64 | ||
| }{ | ||
| // {0.00, 0.0}, // oddly, sample rate = 0.0 means 1.0, skip test for now. |
There was a problem hiding this comment.
It's because of zero-value for floats. I had no idea how to reasonably go around it ¯_(ツ)_/¯
There was a problem hiding this comment.
There are a few possibilities like Functional Options; use pointers to detect zero-value vs not-set; etc. All with pros and cons.
Since this is part of sentry.Init, any change would break every example code out there :/
kamilogorek
approved these changes
Jun 26, 2020
|
Nice patch! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original motivation was noticing that calling
rand.NewinClient.processEventwas odd.Writing a small benchmark reviewed that calling
os.Hostname()for every event was also wasteful.This PR addresses only those two points and is not meant as a comprehensive optimization effort.
The two changes combined result in this
benchstatoutput:go version go1.14.3 darwin/amd64
go version go1.14.4 linux/amd64