Node errors are very slow to create

ronag commented 1 year ago

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 commented 1 year ago

@mcollina

anonrig commented 1 year ago

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

RafaelGSS commented 1 year ago

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

mcollina commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

benjamingr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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

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 commented 10 months ago

Created now also

https://github.com/nodejs/node/pull/49956

as first step to improve SystemError instanciation.

Uzlopak commented 9 months ago

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 commented 9 months ago

Sooo.

Created my proposal: https://github.com/nodejs/node/pull/49990

nodejs / performance

Node errors are very slow to create #40