-
Notifications
You must be signed in to change notification settings - Fork 984
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
feat(chain): schedule handlers to the next tick #1798
Conversation
@@ -161,7 +161,9 @@ function call(handler, err, req, res, _next) { | |||
return; | |||
} else if (!hasError && arity < 4) { | |||
// request-handling middleware | |||
handler(req, res, next); | |||
process.nextTick(function nextTick() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you run the benchmarks with this PR? Consider this implementation instead:
process.nextTick(handler, req, res, next);
Looking at the implementation of nextTick
with Julien, it isn't clear which would be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I run, looks like no effect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal would be cleaner flamegraphs not faster execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Wondering if this should be an option? Otherwise it's a breaking change, I think.
@@ -161,7 +161,9 @@ function call(handler, err, req, res, _next) { | |||
return; | |||
} else if (!hasError && arity < 4) { | |||
// request-handling middleware | |||
handler(req, res, next); | |||
process.nextTick(function nextTick() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this enough to reset the FG stack? Or do we need setImmediate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we mean by "FG stack"? If the question is whether the handler
call's stack frame would not have the previous handler as a parent stack frame, the answer is yes, it would not have the previous handler as a parent stack frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah Peter clarified that FG is for Flame Graphs :)
@DonutEspresso why do you think it's a breaking change? It doesn't affect any of the public APIs or execution order. |
If it's nextTick it's probably not breaking - if it's setImmediate then I'm thinking in a worst case scenario this could cause the queueing behavior to change, e.g., the number of inflight requests may increase and latency gets amortized across the requests in the queue. |
Schedule handlers to nextTick for cleaner Flamegraphs.
Shouldn't have an effect on performance: