Node errors are very slow to create

[ronag]

Unfortunately, it's becoming increasingly common that errors are used as part of normal control flow.

• AbortController throws an AbortError as part of cancellation logic.
• A lot of web frameworks use throwing errors as part of request response logic using e.g. https://www.npmjs.com/package/http-errors. Hence, a normal 404 response can have nontrivial overhead due to to creating and throwing an error instead of simply doing res.statusCode = 404; res.end(); return;.
• Web specs use throwing errors as a normal flow control mechanism (e.g. WebStreams)

Hence the current state of constructing errors in Node is not optimal. The errors are heavily enriched with lots of helpful information, however at a huge performance cost. We need to investigate what can be done to improve this and discuss possible compromises.

[ronag]

@mcollina

[anonrig]

I’m aware of this, and it’s on my to-do list. Appreciate any experiences and/or thoughts on this!

[RafaelGSS]

I want to talk about it next meeting. I have some ideas.

[mcollina]

This is really problematic. I would recommend we include also @jasnell and maybe we schedule a custom meeting for this. There is a lot to talk about.

[aduh95]

FWIW I have an open PR that makes Error creation slower, but also less vulnerable. I guess it'd be nice to see if there's a way to make both things happen or if those are two conflicting goals.

[BridgeAR]

I am currently looking into improving the performance. So far I have identified two things that we are able to improve.

We are able to move some checks from the runtime to the setup time. This is however only a small part of the overall performance overhead. One big bottleneck is ErrorCaptureStackTrace and this is a tricky part. We might be able to remove that.

Besides the actual performance bottleneck, the implementation itself has become difficult to maintain by now as we have lots of very similar functions that create things slightly different. I am working on unifying that but it is going to take some time to do this right.

[BridgeAR]

One note on AbortError: these are "simple" errors and they should be on par with other errors.
What we can improve is to remove the stack trace as it should likely not contain any useful data. Creating the stack trace is pretty much always the biggest overhead for errors.

[mcollina]

@BridgeAR a quick analysis on my end showed that the bottleneck for generic errors (such as TypeError) comes from our override. I don't think we should override the "standard" errors at all.

[jasnell]

Yeah the internal/errors.js situation is... Unfortunate. Tons more complexity has evolved there than was originally intended.

[benjamingr]

We can't remove stack traces usually (in stuff like AbortError) as it significantly harms debugging. The workaround is to capture them lazily like "regular" errors and move to more standard errors like @mcollina suggests IMO.

[BridgeAR]

I already implemented some first improvements locally:

                                                                        confidence improvement accuracy (*)   (**)   (***)
error/hidestackframes.js nested=0 n=10000 type='direct-call-noerr'              **     10.55 %       ±7.38% ±9.82% ±12.78%
error/hidestackframes.js nested=0 n=10000 type='direct-call-throw'             ***     86.07 %       ±4.43% ±5.92%  ±7.77%
error/hidestackframes.js nested=0 n=10000 type='hide-stackframes-noerr'                 2.42 %       ±6.27% ±8.35% ±10.88%
error/hidestackframes.js nested=0 n=10000 type='hide-stackframes-throw'        ***     85.11 %       ±4.05% ±5.39%  ±7.01%
error/hidestackframes.js nested=1 n=10000 type='direct-call-noerr'                      4.10 %       ±4.73% ±6.29%  ±8.19%
error/hidestackframes.js nested=1 n=10000 type='direct-call-throw'             ***     97.84 %       ±3.46% ±4.61%  ±6.00%
error/hidestackframes.js nested=1 n=10000 type='hide-stackframes-noerr'                -0.35 %       ±4.18% ±5.56%  ±7.24%
error/hidestackframes.js nested=1 n=10000 type='hide-stackframes-throw'        ***     91.28 %       ±3.56% ±4.76%  ±6.23%
error/node-error.js type='node' n=100000                                       ***    167.19 %       ±7.47% ±9.99% ±13.11%
error/node-error.js type='regular' n=100000                                             0.15 %       ±4.17% ±5.55%  ±7.23%

[BridgeAR]

I checked AbortError again and it seems like the stack trace is needed in most cases to allow users to identify the point where they called abort() or similar.

There are a few places where it's possible to remove the stack frames such as when running in a timeout (AbortSignal.timeout(ms)). Besides that, I have only found a case in child process that should actually create the error earlier to include the proper stack trace.

The errors are just slow as long as we have to collect the stack frames and we should probably keep that behavior as it is.

[H4ad]

Just to keep things tracked, currently, we have these PRs addressing this issue:

• errors: improve error creation performance node#46648
• errors: improve formatList in errors.js node#49642
• errors: improve performance of classRegExp in errors.js node#49643
• errors: improve performance of NodeError instantiation node#49654
• errors: improve performance of determineSpecificType node#49696
• errors: fix stacktrace of SystemError, add more tests and benchmark node#49956
• errors: improve hideStackFrames node#49990

Except for the first, all of them were made by @Uzlopak.

The first one brings a lot of improvements but hasn't had any activity in months, @BridgeAR do you need any help?

Probably these new changes made by Uzlopak will bring a lot of conflicts, and maybe some of the optimizations will overlap with the changes made by BridgeAR, if we don't receive updates from BridgeAR, I suggest creating new smaller PRs using the first PR as a base just to not lose these big improvements.

@anonrig I think it's worth adding this issue back to the agenda just to discuss current improvements (if that's enough to close this issue) and what we can do with #46648

[Uzlopak]

Created now also

nodejs/node#49956

as first step to improve SystemError instanciation.

[Uzlopak]

I think i found a somewhat elegant way to change hideStackframe so that we dont need to patch the name of the function, and the need to handle those in prepareStackTrace. It should then also make the use of captureLargerStackTrace obsolete.

On the long run, I try to avoid providing a custom prepareStackTrace, and stack trace output should then be based solely on v8. But i cant promise that.

One obstacle at a time.

Stay tuned.

[Uzlopak]

Sooo.

Created my proposal: nodejs/node#49990