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

console,internal/errors: improve error perf #13743

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

This increases the performance for console.trace and creating a new NodeError by factor 2-3.

Newer v8 versions exclude the constructor from the stack trace
so the recalculation of the trace can be avoided.

Using a object instead of an Error is sufficient for console.trace as the Error
itself won't be used but only the stack trace that would
otherwise be created twice.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

console, internal/errors

@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jun 17, 2017
lib/console.js Outdated
err.message = util.format.apply(null, args);
Error.captureStackTrace(err, trace);
this.error(err.stack);
const o = {
Copy link
Contributor

@mscdex mscdex Jun 17, 2017

Choose a reason for hiding this comment

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

Perhaps we can keep the more descriptive variable name here (err)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's not really a typical error anymore but I do not have any strong feelings.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. Commit message-wise, I'd prefer if it addresses what is changed rather than what are the results of the changes.

BridgeAR added 2 commits June 19, 2017 17:41
Using a object instead of an Error is sufficient as the Error
itself won't be used but only the stack trace that would
otherwise be created twice.

This improves the overall .trace() performance.
Newer v8 versions exclude the constructor from the stack trace
so the recalculation of the trace can be avoided.
@BridgeAR BridgeAR force-pushed the improve-error-perf branch from 932788a to e16faad Compare June 19, 2017 15:42
@BridgeAR
Copy link
Member Author

I changed the commit messages.

@@ -28,7 +28,6 @@ function makeNodeError(Base) {
constructor(key, ...args) {
super(message(key, args));
this[kCode] = key;
Error.captureStackTrace(this, NodeError);
Copy link
Member

Choose a reason for hiding this comment

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

I'm -1 on removing this line. The fact that NodeError is being used in here should not be visible to the end user. This intentionally removes that frame from the stack trace to provide that transparency.

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 frame is not visible because newer v8 versions remove the constructor frame by default.
That's the reason why I removed the Error.captureStackTrace call. Do you maybe know a better description for the commit message to make it clearer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I checked all other occurrences as well and they are always needed besides here.

Copy link
Member

Choose a reason for hiding this comment

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

oh nice... I knew that change was coming but I wasn't expecting it until 6.x for some reason. :-) ... that's a happy surprise. I withdraw the objection then!

@refack
Copy link
Contributor

refack commented Jun 22, 2017

@refack
Copy link
Contributor

refack commented Jun 22, 2017

Landed in 70b31ad and 0401754

@refack refack closed this Jun 22, 2017
refack pushed a commit that referenced this pull request Jun 22, 2017
Using a object instead of an Error is sufficient as the Error
itself won't be used but only the stack trace that would
otherwise be created twice.

This improves the overall .trace() performance.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
refack pushed a commit that referenced this pull request Jun 22, 2017
Newer v8 versions exclude the constructor from the stack trace
so the recalculation of the trace can be avoided.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor

refack commented Jun 22, 2017

Quick sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/6833/

addaleax pushed a commit that referenced this pull request Jun 24, 2017
Using a object instead of an Error is sufficient as the Error
itself won't be used but only the stack trace that would
otherwise be created twice.

This improves the overall .trace() performance.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 24, 2017
Newer v8 versions exclude the constructor from the stack trace
so the recalculation of the trace can be avoided.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@addaleax addaleax mentioned this pull request Jun 24, 2017
addaleax pushed a commit that referenced this pull request Jun 29, 2017
Using a object instead of an Error is sufficient as the Error
itself won't be used but only the stack trace that would
otherwise be created twice.

This improves the overall .trace() performance.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 29, 2017
Newer v8 versions exclude the constructor from the stack trace
so the recalculation of the trace can be avoided.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Using a object instead of an Error is sufficient as the Error
itself won't be used but only the stack trace that would
otherwise be created twice.

This improves the overall .trace() performance.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
Newer v8 versions exclude the constructor from the stack trace
so the recalculation of the trace can be avoided.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Using a object instead of an Error is sufficient as the Error
itself won't be used but only the stack trace that would
otherwise be created twice.

This improves the overall .trace() performance.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Newer v8 versions exclude the constructor from the stack trace
so the recalculation of the trace can be avoided.

PR-URL: #13743
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@BridgeAR BridgeAR deleted the improve-error-perf branch April 15, 2018 19:57
@BridgeAR BridgeAR restored the improve-error-perf branch April 15, 2018 19:57
@BridgeAR BridgeAR deleted the improve-error-perf branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants