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

feat(chain): schedule handlers to the next tick #1798

Merged
merged 1 commit into from Jul 31, 2019
Merged

Conversation

hekike
Copy link
Member

@hekike hekike commented Jul 26, 2019

Schedule handlers to nextTick for cleaner Flamegraphs.
Shouldn't have an effect on performance:

? Do you want to track progress? No
? Do you want to compare HEAD with the stable release (7.2.0)? Yes
? Do you want to run all benchmark tests? Yes
? How many connections do you need? 100
? How many pipelining do you need? 10
? How long does it take? 30
---- response-json ----
⠋ Started head/response-json
✔ Results saved for head/response-json
✔ Results saved for stable/response-json
response-json throughput:
{
    "significant": "",
    "equal": true
}

---- response-text ----
✔ Results saved for head/response-text
✔ Results saved for stable/response-text
response-text throughput:
{
    "significant": "",
    "equal": true
}

---- router-heavy ----
✔ Results saved for head/router-heavy
✔ Results saved for stable/router-heavy
router-heavy throughput:
{
    "significant": "***",
    "equal": true
}

---- middleware ----
✔ Results saved for head/middleware
✔ Results saved for stable/middleware
middleware throughput:
{
    "significant": "",
    "equal": true
}

@@ -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() {
Copy link

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

@DonutEspresso DonutEspresso left a 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() {
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

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.

Copy link
Contributor

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 :)

@hekike
Copy link
Member Author

hekike commented Jul 26, 2019

@DonutEspresso why do you think it's a breaking change? It doesn't affect any of the public APIs or execution order.

@DonutEspresso
Copy link
Member

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.

@hekike hekike merged commit 806ed71 into master Jul 31, 2019
@hekike hekike deleted the feat/handle-next-tick branch July 31, 2019 22:07
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.

None yet

4 participants