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/

benjamingr

I'm traveling and won't have time to review fully but please let's all be super careful landing these sort of changes and this one (changing stack creation and errors) in particular. They have a very high breakage potential.

Other than that great work Ruben!

Directly concat the necessary information to a string instead of allocating intermediate arrays and iterating over the array more than once. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

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

The assert module should only be used by users, not by Node.js internally. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

Instead of providing a similar functionality, just reuse the other method. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

The arguments check is moved to the error class setup time instead of error instance creation time. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

This improves the performance by removing the extra stack trace collection step. Instead, just change the maximum stack trace limit to the current limit plus 25. The arbitrary 25 is used as arbitrary upper bound to prevent huge overhead in recursive methods. The limit should normally not be reached and should therefore suffice. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

Currently, already collected user stack frames are removed during stack frame serialization to adhere to the defined maximum stack frame limit. This limit is mainly there to prevent extra cpu overhead while gathering the stack frames. Removing stack frames after already collecting them won't improve the performance and only limit the debuggability for users later. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

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:51Z

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/

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

Open

benjamingr reviewed Feb 28, 2023

View reviewed changes

BridgeAR added 15 commits March 2, 2023 01:04

errors: improve hideInternalStackFrames performance

ed1a664

Directly concat the necessary information to a string instead of allocating intermediate arrays and iterating over the array more than once. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

errors: reuse addNumericSeparator from internal inspect

d21f72b

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

lib: make sure internal assertions use internal/assert

b287852

The assert module should only be used by users, not by Node.js internally. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

net: reuse existing aggregateTwoErrors functionality

c4f3517

Instead of providing a similar functionality, just reuse the other method. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

errors: minor performance improvement

0d72d40

The arguments check is moved to the error class setup time instead of error instance creation time. Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

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

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

benjamingr left a comment

nodejs-github-bot commented Mar 2, 2023

jasnell Mar 2, 2023

BridgeAR Mar 2, 2023

BridgeAR Mar 2, 2023

aduh95 Mar 2, 2023 •

edited

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

errors: improve error creation performance #46648

Are you sure you want to change the base?

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

benjamingr left a comment

Choose a reason for hiding this comment

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

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

aduh95 Mar 2, 2023 •

edited