Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pool timers to avoid allocations #297

Merged
merged 4 commits into from
Jun 1, 2017
Merged

Pool timers to avoid allocations #297

merged 4 commits into from
Jun 1, 2017

Conversation

nussjustin
Copy link
Contributor

Requests are somewhat allocation heavy. This should help a bit. Also has a small positive effect on requests performance (more for the old request style).

Benchstat (base 9147cfa):

name                              old time/op    new time/op    delta
Request-8                           43.9µs ± 5%    43.4µs ± 1%     ~     (p=0.690 n=5+5)
OldRequest-8                        60.0µs ± 3%    57.5µs ± 2%   -4.30%  (p=0.008 n=5+5)

name                              old alloc/op   new alloc/op   delta
Request-8                             885B ± 0%      693B ± 0%  -21.71%  (p=0.008 n=5+5)
OldRequest-8                        2.63kB ± 0%    2.44kB ± 0%   -7.27%  (p=0.000 n=5+4)

name                              old allocs/op  new allocs/op  delta
Request-8                             17.0 ± 0%      14.0 ± 0%  -17.65%  (p=0.008 n=5+5)
OldRequest-8                          55.0 ± 0%      52.0 ± 0%   -5.45%  (p=0.008 n=5+5)

Other benchmarks show no changes.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 94.376% when pulling 6d0118b on nussjustin:timer-pool into 9147cfa on nats-io:master.

@nussjustin
Copy link
Contributor Author

Test failure is unrelated and also happens on master (though only 2 times in 20 local runs, 10 with and 10 without -race).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 94.492% when pulling 4330ddd on nussjustin:timer-pool into 9147cfa on nats-io:master.

@nussjustin
Copy link
Contributor Author

Since this works with sync.Pool testing is somewhat tricky. The test would need to disable GC (and reenable afterwards) and set GOMAXPROCS to 1 (and reset it afterwards) to prevent the pool from being emptied and to avoid rescheduling onto another P.

If you want I can push a test.

@tylertreat
Copy link
Contributor

lgtm. Would be nice to have some basic tests around timerPool, though I agree it's a bit tricky to test with the GC behavior.

@nussjustin
Copy link
Contributor Author

Pushed a new commit with a test that should work.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 94.492% when pulling 6a1a909 on nussjustin:timer-pool into 9147cfa on nats-io:master.

@nussjustin
Copy link
Contributor Author

Ah, okay. sync.Pool will randomly drop values when compiling with -race to prevent assuming it's behaviour. I changed the test to just check the behaviour of the timer instead.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 94.492% when pulling 1630db7 on nussjustin:timer-pool into 9147cfa on nats-io:master.

@tylertreat
Copy link
Contributor

+1

@derekcollison derekcollison merged commit d4ca4c8 into nats-io:master Jun 1, 2017
@nussjustin nussjustin deleted the timer-pool branch June 1, 2017 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants