-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Safely ignore error stack trace #130
Comments
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 |
But, wouldn't it show no stacktraces at all? If so, this is unlikely to happen due to many reasons. |
Yep, it will be empty and will just print the error message.
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 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. |
I wonder if there are any hot paths which:
If so, they could benefit greatly from this (since it is a ~10x speedup). |
@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 |
Remember that in most cases, removing a stack trace may do more harm than good. They are quite useful. |
Agreed, the point here is that for |
@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): CanadaHonk/node@7a774cd The idea shows some potential though, if a place was found which would have a real world benefit. |
@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. |
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? |
@tushar32 It can help but you need to throw a lot of exceptions to this micro-opmization to be relevant. |
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. |
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.) |
Maybe you find here some info? |
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 #40 |
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:
Code
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:
The text was updated successfully, but these errors were encountered: