errors: improve error creation performance #46648

BridgeAR · 2023-02-14T12:00:27Z

This is a first step to significantly faster NodeErorrs. The code itself should also be cleaned up in a following PR but it already became bigger as I worked upon different parts related to errors in Node.js.

This does change stack traces for users in few cases: originally we added extra stack frames to compensate for stack frames we want to hide. The mechanism as it was implemented had a high cost on all Node.js errors in case they were created in a deeply nested function. Stack frames that are removed are now not going to be compensated for anymore. I am going to think about other ways to do this later on again.

Refs: nodejs/performance#40

                                                                        confidence improvement accuracy (*)   (**)   (***)
error/hidestackframes.js nested=0 n=10000 type='direct-call-noerr'                      0.52 %       ±5.47% ±7.30%  ±9.56%
error/hidestackframes.js nested=0 n=10000 type='direct-call-throw'             ***     81.31 %       ±7.21% ±9.66% ±12.72%
error/hidestackframes.js nested=0 n=10000 type='hide-stackframes-noerr'                 2.66 %       ±6.63% ±8.83% ±11.49%
error/hidestackframes.js nested=0 n=10000 type='hide-stackframes-throw'        ***     79.61 %       ±6.63% ±8.87% ±11.66%
error/hidestackframes.js nested=1 n=10000 type='direct-call-noerr'                      0.21 %       ±3.57% ±4.76%  ±6.19%
error/hidestackframes.js nested=1 n=10000 type='direct-call-throw'             ***     90.10 %       ±5.93% ±7.93% ±10.40%
error/hidestackframes.js nested=1 n=10000 type='hide-stackframes-noerr'                -2.34 %       ±4.07% ±5.42%  ±7.06%
error/hidestackframes.js nested=1 n=10000 type='hide-stackframes-throw'        ***     85.71 %       ±6.18% ±8.23% ±10.73%
error/node-error.js type='node' n=100000                                       ***    166.64 %       ±7.01% ±9.39% ±12.35%
error/node-error.js type='regular' n=100000                                            -2.29 %       ±2.98% ±3.97%  ±5.16%

nodejs-github-bot · 2023-02-14T12:00:32Z

anonrig

Amazing!

lib/assert.js

anonrig · 2023-02-14T13:41:21Z

cc @nodejs/performance

BridgeAR · 2023-02-14T13:58:00Z

I checked how much potential is left for further improvements and it turns out this is almost an ideal state.

Our main overhead is due to creating subclasses. ~50% over the most simple errors without subclass. The other ~15-20% overhead are related to our error message creation. It would actually be possible to roughly cut the latter costs into half by creating a formatPreparation method that pre calculates states and caches these for the later usage. It would however be a non-trivial code part and I believe this is already pretty nice.

ronag · 2023-02-14T14:05:54Z

@BridgeAR any chance to further explicitly optimise AbortError as that often occurs also in non exceptional logic (i.e. fast paths).

BridgeAR · 2023-02-14T14:21:08Z

@ronag the current implementation for AbortErrors is pretty much the bare minimum of a regular error.

The question is what are these used for? If their stack trace is important, we can't improve them further. If not, then there's room for improvement by hiding the frames (which are ~75% of all CPU cycles for a regular error).
To me it seems like the main stack frames that would be relevant for users are mostly coming from the signal.reason. These stack frames would not be touched, only the newly created AbortError ones. I had a short discussion if they are important or not with @benjamingr and I have the feeling the answer is: it depends.
If you therefore have some concrete spots that need some love, please let me know and I am going to check if it's possible to further minimize the overhead.

jasnell · 2023-02-14T15:18:27Z

I'm not going to have time to review the entire PR this week but the changes in internal/errors.js LGTM.

lib/internal/errors.js

lib/assert.js

lib/internal/console/constructor.js

lib/internal/errors.js

lib/internal/source_map/prepare_stack_trace.js

lib/internal/streams/readable.js

lib/internal/test_runner/utils.js

mcollina

lgtm, this is truly amazing

RafaelGSS

RSLGTM. Awesome work!

VoltrexKeyva

RSLGTM, this is absolutely incredible!

JakobJingleheimer

🙌

general nit: there are several large numbers in messages for humans, but they're inconsistently delimited. for example 'The value of "port" is out of range. It must be >= 0 && <= 65535. Received 99_999'. Why not delimit all human-friendly numbers?

benchmark/error/node-error.js

lib/internal/streams/readable.js

nodejs-github-bot · 2023-02-17T20:34:29Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49658/

This simplifies the implementation of the OUT_OF_RANGE error by reusing the current inspect functionality. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

The former implementation over allocated stack frames during error creation to substitute for internal frames that should be hidden to the user. This could have caused a significant performance overhead in deeply nested code. As side effect also simplify the setting of stackTraceLimits and improve the error related benchmarks. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

This makes sure all error instances use `new` to call the constructor. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

The improvement comes from the following changes: 1. Less properties are defined on the error instances, especially no Symbol. 2. The check if something is a NodeError or not is not needed anymore. 3. The setup of NodeError now makes sure that all heavy lifting is done up front instead of doing that during instance creation. To align with the WPT tests, the NodeErrors must have their constructor set to their base class. This was not the case for SystemErrors and is now aligned. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

This improves the error recreation performance due to limiting the stack trace that is generated and it makes sure NodeErrors only set properties inside of the actual error method. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

This fixes the issue that multiple prepareStackTrace functions existed that had to duplicate code in multiple spots. Instead, the diverging part is now replaced during runtime. This reduces the code overhead and makes sure there is no duplication. It also fixes the issue that `overrideStackTrace` usage would not adhere to the `hideStackFrame` calls. Frames are now removed before passing them to the override method. A second fix is for the repl: the stack frames passed to a user defined Error.prepareStackTrace() method are now in the correct order. As drive-by it also improves the performance for repl errors marginally. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

The function calls are more expensive than using the error printf format style. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot · 2023-03-02T00:29:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50142/

jasnell · 2023-03-02T00:47:50Z

lib/internal/errors.js

+    try {
+      Error.stackTraceLimit = limit;
+    } catch {
+      stackTraceLimitWritable = -1;


Nit: A comment here on why this would throw would be helpful for future generations ;-)

I suggest to address that in a follow-up PR? :)

// Setting the property in strict mode would cause this to throw an error // while using the --frozen-intrinsics flag due to the writable attribute // being set to false.

I actually wonder, if we want to keep that property writable even when using frozen intrinsics.
@aduh95 @bmeck you might have an opinion about that?

To fill the contract of --frozen-intrinsics (opt-out of all possible prototype mutation), we need the property to be non-configurable, it's true that it could still be writable without introducing too much risk I believe. (but the flag name would be kinda lying, not the end of the world of course, but a bit harder to document in simple terms)

But even if we did that, we would probably still want the try catch in case some user code has changed this to non-writable for some reason, unless it's introducing too much maintenance burden. wdyt?

nodejs-github-bot · 2023-03-02T12:51:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50153/

nodejs-github-bot · 2023-03-02T13:50:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50154/

nodejs-github-bot · 2023-03-02T15:50:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50155/

nodejs-github-bot · 2023-03-02T22:55:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/50174/

anonrig · 2023-04-12T14:12:41Z

@BridgeAR It seems like there are some conflicts on the PR. Would you like to rebase it?

Uzlopak · 2023-09-14T09:05:43Z

I would actually say, that we should split this PR in multiple PRs so that it can be easier reviewed.

github-actions · 2024-05-11T13:12:21Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

github-actions · 2024-06-12T00:01:37Z

Closing this because it has stalled. Feel free to reopen if this issue/PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

BridgeAR requested review from mcollina, jasnell, anonrig and ronag February 14, 2023 12:00

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 14, 2023

BridgeAR force-pushed the improve-error-performance branch from 846a306 to 15854a8 Compare February 14, 2023 12:21

anonrig approved these changes Feb 14, 2023

View reviewed changes

targos reviewed Feb 14, 2023

View reviewed changes

lib/assert.js Outdated Show resolved Hide resolved

ronag approved these changes Feb 14, 2023

View reviewed changes

anonrig added the performance Issues and PRs related to the performance of Node.js. label Feb 14, 2023

aduh95 reviewed Feb 14, 2023

View reviewed changes

lib/internal/errors.js Outdated Show resolved Hide resolved

lib/internal/errors.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

ljharb reviewed Feb 14, 2023

View reviewed changes

mcollina approved these changes Feb 14, 2023

View reviewed changes

RafaelGSS approved these changes Feb 14, 2023

View reviewed changes

aduh95 mentioned this pull request Feb 14, 2023

tools: fix bug in prefer-primordials lint rule #46659

Merged

VoltrexKeyva approved these changes Feb 15, 2023

View reviewed changes

JakobJingleheimer approved these changes Feb 15, 2023

View reviewed changes

benchmark/error/node-error.js Outdated Show resolved Hide resolved

benchmark/error/node-error.js Outdated Show resolved Hide resolved

lib/internal/streams/readable.js Outdated Show resolved Hide resolved

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 17, 2023

This was referenced Feb 18, 2023

CI Reliability 2023-02-18 nodejs/reliability#499

Open

CI Reliability 2023-02-19 nodejs/reliability#500

Open

anonrig mentioned this pull request Feb 20, 2023

AbortSignal pattern is slow nodejs/performance#44

Closed

BridgeAR added 8 commits March 2, 2023 01:06

errors: format more numbers with numeric separator

4ddd43f

This simplifies the implementation of the OUT_OF_RANGE error by reusing the current inspect functionality. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

lib: add new to construct errors

6ec603c

This makes sure all error instances use `new` to call the constructor. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

errors: remove unused ERR_INVALID_RETURN_PROPERTY error

ee8e40f

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

errors: faster ERR_MODULE_NOT_FOUND error

40ab9c3

The function calls are more expensive than using the error printf format style. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR force-pushed the improve-error-performance branch from c6ec54f to 40ab9c3 Compare March 2, 2023 00:24

BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 2, 2023

jasnell reviewed Mar 2, 2023

View reviewed changes

This was referenced Mar 3, 2023

CI Reliability 2023-03-03 nodejs/reliability#512

Open

CI Reliability 2023-03-04 nodejs/reliability#513

Open

CI Reliability 2023-03-05 nodejs/reliability#514

Open

CI Reliability 2023-03-06 nodejs/reliability#515

Open

JakobJingleheimer approved these changes Mar 9, 2023

View reviewed changes

anonrig mentioned this pull request Apr 12, 2023

request: expose Node's error system #47521

Closed

H4ad mentioned this pull request Sep 29, 2023

Node errors are very slow to create nodejs/performance#40

Open

aduh95 added the stalled Issues and PRs that are stalled. label May 11, 2024

github-actions bot closed this Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: improve error creation performance #46648

errors: improve error creation performance #46648

BridgeAR commented Feb 14, 2023

nodejs-github-bot commented Feb 14, 2023

anonrig left a comment

anonrig commented Feb 14, 2023

BridgeAR commented Feb 14, 2023

ronag commented Feb 14, 2023

BridgeAR commented Feb 14, 2023

jasnell commented Feb 14, 2023

mcollina left a comment

RafaelGSS left a comment

VoltrexKeyva left a comment

JakobJingleheimer left a comment

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Mar 2, 2023

jasnell Mar 2, 2023

BridgeAR Mar 2, 2023

BridgeAR Mar 2, 2023

aduh95 Mar 2, 2023 •

edited

Loading

nodejs-github-bot commented Mar 2, 2023

nodejs-github-bot commented Mar 2, 2023

nodejs-github-bot commented Mar 2, 2023

nodejs-github-bot commented Mar 2, 2023

anonrig commented Apr 12, 2023

Uzlopak commented Sep 14, 2023

github-actions bot commented May 11, 2024

github-actions bot commented Jun 12, 2024

errors: improve error creation performance #46648

errors: improve error creation performance #46648

Conversation

BridgeAR commented Feb 14, 2023

nodejs-github-bot commented Feb 14, 2023

anonrig left a comment

Choose a reason for hiding this comment

anonrig commented Feb 14, 2023

BridgeAR commented Feb 14, 2023

ronag commented Feb 14, 2023

BridgeAR commented Feb 14, 2023

jasnell commented Feb 14, 2023

mcollina left a comment

Choose a reason for hiding this comment

RafaelGSS left a comment

Choose a reason for hiding this comment

VoltrexKeyva left a comment

Choose a reason for hiding this comment

JakobJingleheimer left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 17, 2023

nodejs-github-bot commented Mar 2, 2023

jasnell Mar 2, 2023

Choose a reason for hiding this comment

BridgeAR Mar 2, 2023

Choose a reason for hiding this comment

BridgeAR Mar 2, 2023

Choose a reason for hiding this comment

aduh95 Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 2, 2023

nodejs-github-bot commented Mar 2, 2023

nodejs-github-bot commented Mar 2, 2023

nodejs-github-bot commented Mar 2, 2023

anonrig commented Apr 12, 2023

Uzlopak commented Sep 14, 2023

github-actions bot commented May 11, 2024

github-actions bot commented Jun 12, 2024

aduh95 Mar 2, 2023 •

edited

Loading