Reduce apply() calls to improve call times. #765
Conversation
Reduces fire() call time by about 50%, and fallback call time by 30% fire() x 134,900 ops/sec ±42.25% (21 runs sampled) fire() error x 68,030 ops/sec ±31.60% (8 runs sampled) fire() x 286,626 ops/sec ±28.73% (56 runs sampled) fire() error x 99,637 ops/sec ±2.48% (10 runs sampled)
| @@ -582,7 +580,8 @@ class CircuitBreaker extends EventEmitter { | |||
| const err = buildError('The circuit has been shutdown.', 'ESHUTDOWN'); | |||
| return Promise.reject(err); | |||
| } | |||
| const args = Array.prototype.slice.call(rest); | |||
|
|
|||
| const args = rest.slice(); | |||
There was a problem hiding this comment.
rest arguments become [] when no arguments provided, so Array.prototype.slice is guarding nothing.
There was a problem hiding this comment.
I think i remember why Array.prototype.slice was used. originally we used the arguments parameter which array like but not an array, so that whole thing was needed. when it was convereted to rest params, we just forgot to take it out
There was a problem hiding this comment.
Sounds about right
There was a problem hiding this comment.
Mostly I was commenting to say "I thought about undefined"
|
This addresses questions asked and answered in #758 |
Pull Request Test Coverage Report for Build 5095893721
💛 - Coveralls |
|
Talking it out with some coworkers this morning, I mentioned I was a little surprised by how effective this PR was. I was expecting about 2/3rds of this improvement at the outside. What I'm thinking is that the apply/rest performance discrepancy may have increased in Node 16, when V8 changed how it handled function arguments (specifically to speed up calls with the 'wrong' number of args). I looked at this when rest parameters were first introduced and while it was helpful, it wasn't 'drop everything' level of performance gap. At a guess, I'm thinking the average overhead for regular calls went down compared to apply(), anonymous functions versus arrow functions, functions that use arguments, or some combination of the three. |
|
just released 8.0.1 |
This brings opossum.fire() roughly in line with hystrixjs.execute()
Reduces fire() call time by about 50%, and fallback call time by 30%