nodejs / performance

Node.js team focusing on performance
MIT License
371 stars 7 forks source link

Safely ignore error stack trace #130

Open H4ad opened 8 months ago

H4ad commented 8 months ago

I saw this library https://github.com/isaacs/catcher and I decided to wrote a benchmark:

Doing some changes on bench/error.js, the result was:

name ops/sec samples
Error 337,720 64
Error (stackTraceLimit=0) 3,284,296 94
NodeError 283,026 99
NodeError (stackTraceLimit=0) 3,280,933 95
NodeError Range 214,825 92
NodeError Range (stackTraceLimit=0) 3,313,990 99
Code ```js const { createBenchmarkSuite } = require('../common') const suite = createBenchmarkSuite('Node.js Error') suite .add('Error', function () { try { new Error('test') } catch (e) { } }) .add('Error (stackTraceLimit=0)', function () { const originalStackTraceLimit = Error.stackTraceLimit Error.stackTraceLimit = 0 try { new Error('test') } catch (e) { } finally { Error.stackTraceLimit = originalStackTraceLimit; } }) .add('NodeError', function () { try { new TypeError('test') } catch (e) { } }) .add('NodeError (stackTraceLimit=0)', function () { const originalStackTraceLimit = Error.stackTraceLimit Error.stackTraceLimit = 0 try { new TypeError('test') } catch (e) { } finally { Error.stackTraceLimit = originalStackTraceLimit; } }) .add('NodeError Range', function () { try { new RangeError('test') } catch (e) { } }) .add('NodeError Range (stackTraceLimit=0)', function () { const originalStackTraceLimit = Error.stackTraceLimit Error.stackTraceLimit = 0 try { new RangeError('test') } catch (e) { } finally { Error.stackTraceLimit = originalStackTraceLimit; } }) .run({ async: false }) ```

Based on this assumption, maybe we can find places on Node where we can safely ignore the stackTraceLimit, using this search, I found some places:

Uzlopak commented 8 months ago

Yes, makes sense.

We made imho good experience with this pattern. E.g. https://github.com/fastify/secure-json-parse/blob/4c9c7a6b7490e66406c9847b6583e173ab251f37/index.js#L100

RafaelGSS commented 8 months ago

But, wouldn't it show no stacktraces at all? If so, this is unlikely to happen due to many reasons.

H4ad commented 8 months ago

But, wouldn't it show no stacktraces at all?

Yep, it will be empty and will just print the error message.

If so, this is unlikely to happen due to many reasons.

What do you mean?


Also, I tried to refactor a bunch of places to introduce a function to abstract this pattern and it was not faster, I think for this optimization work, we should manually add Error.stackTraceLimit=0 for each piece of code.

The amount of redundant code I don't think it will worth to add for most cases, but I will continue explore to see if I found some place that worth to add it.

CanadaHonk commented 7 months ago

I wonder if there are any hot paths which:

If so, they could benefit greatly from this (since it is a ~10x speedup).

H4ad commented 7 months ago

@CanadaHonk I tried using a helper function but it didn't help.

I also didn't find any hot paths, just places that could be optimized but they are not critical.

If you want to try this, you should add the try-catch in the same code that you want to optimize, a function will not help.

RafaelGSS commented 7 months ago

Remember that in most cases, removing a stack trace may do more harm than good. They are quite useful.

CanadaHonk commented 7 months ago

Agreed, the point here is that for try { ... } catch { ... }s where we do not use the error, we do not need to waste time generating the stack trace for them (afaict).

CanadaHonk commented 7 months ago

@H4ad I found an example use, although it isn't that useful for real world (I probably won't make it a PR unless requested): https://github.com/CanadaHonk/node/commit/7a774cdb79f14425bfddcc19c028038ef5c8df48

The idea shows some potential though, if a place was found which would have a real world benefit.

H4ad commented 7 months ago

@CanadaHonk I think you should open a PR for this case, the improvements are good even though this is an edge-case and a deprecated function.

tushar32 commented 4 months ago

By setting stackTraceLimit=0 or stackTraceLimit=2 will help to run unit tests faster becasue it throws error in multiple test caces . hence slows down test case?

H4ad commented 3 months ago

@tushar32 It can help but you need to throw a lot of exceptions to this micro-opmization to be relevant.

Uzlopak commented 3 months ago

On my machine: StackTrace off is about 1 mio errors/s and stacktrace at 10 is about 200k errors/s. You wont really feel it in local unit tests.

lemire commented 3 months ago

If you discard entirely stack traces, how much faster does Node run (on some interesting benchmarks)? That is, what is the overhead due to stack traces?

(And, yes, I am aware that Node will keep the stack traces generally speaking... I am asking the question to determine how interesting optimizing stack traces could be.)

Uzlopak commented 3 months ago

@lemire

Maybe you find here some info? https://github.com/fastify/fastify-error/pull/103

H4ad commented 3 months ago

Rafael created a benchmark to track the performance of throwing errors: https://github.com/RafaelGSS/nodejs-bench-operations/blob/main/v21/RESULTS-v21_7_1.md#nodejs-error

If we go with the simplest approach, creating another one with stackTraceLimit=0 should be enough to have an idea.

But we already had good optimizations on throwing errors in the latest releases, you can follow at https://github.com/nodejs/performance/issues/40