Remove an allocation from breaker reserve #8851
Conversation
The previous code captured `b`, which required a closure allocation on each call of Reserve. Pre-allocating the closure as a field removes a little bit of allocation from Reserve, which is worthwhile since Reserve is used at high volume in performance-critical sections of the code.
knative-prow-robot
added
the
size/S
There was a problem hiding this comment.
@julz: 0 warnings.
In response to this:
The previous code captured
b, which required a closure allocation on
each call of Reserve. Pre-allocating the closure as a field removes a
little bit of allocation from Reserve, which is worthwhile since Reserve
is used at high volume in performance-critical sections of the code.Note: simpler options like defining
b.releaseas a method rather than
a closure and returning a method reference inReservestill capture b,
and therefore still allocate :-(./assign @markusthoemmes @vagababov
Benchmarks:
benchmark old ns/op new ns/op delta BenchmarkBreakerReserve/sequential-16 119 92.6 -22.18% BenchmarkBreakerReserve/parallel-16 202 161 -20.30% benchmark old allocs new allocs delta BenchmarkBreakerReserve/sequential-16 1 0 -100.00% BenchmarkBreakerReserve/parallel-16 1 0 -100.00% benchmark old bytes new bytes delta BenchmarkBreakerReserve/sequential-16 16 0 -100.00% BenchmarkBreakerReserve/parallel-16 16 0 -100.00%
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: julz, markusthoemmes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
The following jobs failed:
Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests: |
|
/retest |
The previous code captured
b, which required a closure allocation oneach call of Reserve. Pre-allocating the closure as a field removes a
little bit of allocation from Reserve, which is worthwhile since Reserve
is used at high volume in performance-critical sections of the code.
Note: simpler options like defining
b.releaseas a method rather thana closure and returning a method reference in
Reservestill capture b,and therefore still allocate :-(.
/assign @markusthoemmes @vagababov
Benchmarks: