feat(chain): use nextTick instead of setImmediate #1808
Conversation
BREAKING CHANGE: Using setImmediate has more overhead than nextTick on
Node.js v12. Benchmarks show improvements of >20% when changing
setImmediate to nextTick:
response-json throughput:
{
"significant": "***",
"equal": false,
"wins": "head",
"diff": "28.62%"
}
---- response-text ----
✔ Results saved for head/response-text
✔ Results saved for stable/response-text
response-text throughput:
{
"significant": "***",
"equal": false,
"wins": "head",
"diff": "43.45%"
}
---- router-heavy ----
✔ Results saved for head/router-heavy
✔ Results saved for stable/router-heavy
router-heavy throughput:
{
"significant": "***",
"equal": false,
"wins": "head",
"diff": "30.02%"
}
---- middleware ----
✔ Results saved for head/middleware
✔ Results saved for stable/middleware
middleware throughput:
{
"significant": "***",
"equal": false,
"wins": "head",
"diff": "20.78%"
}
This changes the order some events are processed (nextTick is processed
earlier than setImmediate), thus this can be considered a breaking
change as some edge cases will notice this (as we can see in the test
changes).
There was a problem hiding this comment.
👍 I don't see a need for this to be a breaking change. I'd consider this an internal implementation detail; relying on the timing of the middleware at such a fine level is probably an antipattern (I don't think we have a contract for this). Suspect folks test suites might break with this change like we saw, but wouldn't expect production code to be impacted.
| @@ -2111,7 +2111,7 @@ test('should increment/decrement inflight request count', function(t) { | |||
| CLIENT.get('/foo', function(err, _, res) { | |||
| t.ifError(err); | |||
| t.equal(res.statusCode, 200); | |||
| t.equal(SERVER.inflightRequests(), 1); | |||
| t.equal(SERVER.inflightRequests(), 0); | |||
There was a problem hiding this comment.
I think this is fine. Is this a result of CLIENT and SERVER sharing an event loop? Looking at this change, I don't see how it would cause a problem with our inflight request throttling. I could see the interaction between open request throttling and inflight request throttling changing with this PR, though I don't see a use case for both of those being turned on at the same time.
|
We discussed that we will treat it as implementation details and making a nonbreaking release. |
|
Thanks @mmarchini! |
BREAKING CHANGE: Using setImmediate has more overhead than nextTick on
Node.js v12. Benchmarks show improvements of >20% when changing
setImmediate to nextTick:
This changes the order some events are processed (nextTick is processed
earlier than setImmediate), thus this can be considered a breaking
change as some edge cases will notice this (as we can see in the test
changes).
Pre-Submission Checklist
make prepush