errors: eliminate all overhead for hidden calls

[mmomtchev]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

This in response to #35386

[puzpuzpuz]

Do we still need this function? It looks like it could be replaced with Error.captureStackTrace(ex).

[mmomtchev]

I still need to extend the stack trace size before capturing it. The way the code works is that the stack trace size is set to 0 at the Error creation (so no stack trace is collected), then it manually invokes Error.captureStackTrace(ex). That captureLargerStackTrace(err) is there mainly to centralize at one place extending the stack size and then restoring it back.

[puzpuzpuz]

All invocations of captureLargerStackTrace() are made when Error.stackTraceLimit is already restored from 0 to the original value, so I still don't get why it's needed. Am I missing something?

[mmomtchev]

I need to increase it from the original value to Infinity, since I will be removing some elements and I don't know their number in advance. Then I need to cut off the stack trace to the original value length, so that I can return the exact number of stack trace elements that the user asked for - usually 10.

[puzpuzpuz]

I see now. Then this approach with Infinity may lead to increased memory consumption in user applications, especially when they make standard library calls in deeply nested and/or recursive functions.

It can be demonstrated on the following example:

// comment to avoid crashing
Error.stackTraceLimit = Infinity;
function foo(i) {
    if (i === 32000) {
        throw new Error('bar');
    }
    return foo(i + 1);
}
foo(1);

If you run it with node --max-old-space-size=4 --stack-size=32000 recursive-error.js, it'll crash. Without the Error.stackTraceLimit override, the script stops on unhandled error as expected.

Thus, the current implementation seems to be introducing a potential regressions. We could change Infinity to userStackTraceLimit + N where N is the max depth of standard library validation function calls, but that's a bad approach. Do you have an idea for another workaround here?

[mmomtchev]

I have another idea, but it is even uglier - rename all functions that are to be hidden to the same name and then use the V8 built-in Error.captureStackTrace feature - which is much faster
But it is going to make life even harder for internals developers

[mmomtchev]

In fact, I think that aside from some very edge artificial cases - where the heap is smaller than the stack, we should be ok - if you have a call stack that is so large, that it doesn't fit into the heap - you are bound to run into problems anyway

[puzpuzpuz]

Heap may be large enough to fit the call stack, yet it may not have enough free space to allocate an exception as it also contains other JS objects. So, it's better to speak of the free heap space here.

As for the artificial nature of edge cases, the code snippet above is certainly artificial, but there may be modules and user applications that face regressions introduced with this PR. I'm not 100% sure that there will any and I see the certain value introduced by this PR, so it would be nice to get more opinions here.

cc @nodejs/core

[mmomtchev]

Proceeding by elimination:
There are only two ways of limiting the captured call stack before it is captured - one is by passing a reference of a function to V8 in Error.captureStackTrace. This is not usable without introducing some artificial function that will be able to find on the call stack - this is the old method. In theory, one could imagine some improved solution, where we never add more than once this function - the old method did it multiple times, creating closures at each call.

The other way is limiting the number of entries to a number known in advance - Error.stackTraceLimit. Either we estimate the maximum call stack that the Node internals are allowed to reach, either we try to deduce it in a dynamic way.

[puzpuzpuz]

After some thinking, I would personally prefer option 2, i.e. the original solution, but with a modified "padding" (smth like 32 or even 64). Yes, it may lead to trimmed call stacks in produced errors if the padding is not enough, but at least it won't create unpredictable memory pressure.

[Trott]

Benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/668/

[puzpuzpuz]

Here is the benchmark output:

4:58:50                                                                 confidence improvement accuracy (*)    (**)   (***)
14:58:50  misc/hidestackframes.js n=100000 type='direct-call-noerr'                     -3.01 %       ±4.86%  ±6.48%  ±8.46%
14:58:50  misc/hidestackframes.js n=100000 type='direct-call-throw'             ***    -20.45 %       ±1.86%  ±2.49%  ±3.25%
14:58:50  misc/hidestackframes.js n=100000 type='hide-stackframes-noerr'        ***    137.14 %       ±8.16% ±10.86% ±14.16%
14:58:50  misc/hidestackframes.js n=100000 type='hide-stackframes-throw'        ***     -8.77 %       ±2.59%  ±3.44%  ±4.49%
14:58:50 
14:58:50 Be aware that when doing many comparisons the risk of a false-positive
14:58:50 result increases. In this case there are 4 comparisons, you can thus
14:58:50 expect the following amount of false-positive results:
14:58:50   0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
14:58:50   0.04 false positives, when considering a   1% risk acceptance (**, ***),
14:58:50   0.00 false positives, when considering a 0.1% risk acceptance (***)

As expected, there is a penalty for error-path (even when hideStackFrames() is not in use). On the other hand, success-path' performance improved significantly, which seems to be a good trade-off to me.

[puzpuzpuz]

I also think, it's worth mentioning that this PR will close #35386. Does it make sense?

[BridgeAR]

Thanks for the initiative to improve this! I do have some concerns about the implementation though that I highlighted in my comments.

[mmomtchev]

I also think, it's worth mentioning that this PR will close #35386. Does it make sense?

Where should I reference it? I did it in the commit message? Sorry, I am very new in this repo.

[puzpuzpuz]

Where should I reference it? I did it in the commit message? Sorry, I am very new in this repo.

Sorry, I didn't check the commit message. It's OK to reference the issue there.

[aduh95]

This needs a rebase.

[cclauss]

Please fix merge conflicts.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/34397/

[mmomtchev]

@benjamingr , since your PR, hideStackFrames should return a constructor - which cannot happen if the passed function is an arrow function. It worked before since hideStackFrames generated a new function, but now it is simply a rename
Since you are the only one to use throw new I guess it was an oversight?
Should I change this back to a normal call? In this PR?

throw new lazyDomException needs to become throw lazyDomException

#35911

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/34912/

[aduh95]

@mmomtchev do you want to add comments in the code to resolve the last two open threads? If you prefer, I can add them myself by pushing commits to your branch. Other than that, I think this is ready to land.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/34953/

[aduh95]

Landed in 656ce92

[amcasey]

During module resolution, the TypeScript compiler has to probe many paths that turn out not to exist. To mitigate the cost of creating so many uvExceptions, it sets Error.stackTraceLimit to zero (helpfully suggested here). From profiling, it looks like this change (specifically captureLargerStackTrace node:internal/errors:450 in node v 16.3.0) resulted in a pretty substantial perf regression. Fortunately, newer version of TypeScript use the new throwIfNoEntry flag, mitigating the issue, but older versions of TypeScript are likely to be substantially affected. Have others reported performance issues in this area and, if so, would it be worthwhile to file a bug (I couldn't find an existing one)?

[aduh95]

You should be able to hack your way around it in Node.js v16.0.0+ by making the stackTraceLimit not writable:

Reflect.defineProperty(Error, 'stackTraceLimit', {
  writable: false,
  value: 0,
});

[amcasey]

I have to admit, that would not have occurred to me. 😄

Fortunately, we already have a good mitigation for v16 - we just pass throwIfNoEntry. The problem is more that code we've already shipped (i.e. TypeScript <= 4.1) lacks either mitigation and users will suddenly see a large slowdown. In the repro I happened to notice this in (which, unfortunately, I can't share), the runtime jumps from 30 seconds to 48 seconds when throwIfNoEntry is removed.

(If we end up filing a bug for this, I'll make sure to include a publicly-available sample.)

[BridgeAR]

Ideally, we would have detected it's set to zero and not increased it in that case and only would have set it to it's current limit + a small number for the extra frames that should be removed.

[BridgeAR]

This has caused some performance issues for all errors due to overcollecting stack frames where no frames were meant to be hidden.

[BridgeAR]

With this approach it is actually not possible to limit the performance overhead to the methods that use hideStackFrames() anymore. We always either need to overallocate for all Node.js errors (which may cause a lot of overhead for deeply nested code) or we do not overallocate anything at all and the stack trace may stack empty.