lib: improve error creation performance #24747
Conversation
In case of an error where we only care about a cleaned up stack trace it is cheaper to reset the stack trace limit for the error that is created. That way the stack frames do not have to be computed twice.
nodejs-github-bot
added
errors
|
Do you think V8 could help with this? |
Co-Authored-By: BridgeAR <ruben@bridgewater.de>
I thought about that and the reason why I did not yet do that is that we have very different signatures and we can prevent resetting the numbers in some cases. If we use this pattern more commonly, we could write something like this: function assertSize(size) {
errors.removeFramesFrom(assertSize);
if (typeof size !== 'number') {
throw new ERR_INVALID_ARG_TYPE('size', 'number', size);
}
if (size < 0 || size > kMaxLength) {
throw new ERR_INVALID_OPT_VALUE.RangeError('size', size);
}
errors.resetFrames();
}That way the internal errors could take care of it.
I spoke with @hashseed about this but so far there was no real conclusion (but two issues). I personally would love to get a completely new API that would allow to reduce the code overhead and potentially allow further optimizations in V8. Example APIfunction assertSize(size) {
let err = null;
if (typeof size !== 'number') {
err = new ERR_INVALID_ARG_TYPE('size', 'number', size);
} else if (size < 0 || size > kMaxLength) {
err = new ERR_INVALID_OPT_VALUE.RangeError('size', size);
}
if (err !== null) {
Error.captureStackTrace(err, assertSize);
throw err;
}
}
// If this would instead be written similar to:
function assertSize(size) {
Error.dontSetLowerStackFrames();
if (typeof size !== 'number') {
throw new ERR_INVALID_ARG_TYPE('size', 'number', size);
} else if (size < 0 || size > kMaxLength) {
throw new ERR_INVALID_OPT_VALUE.RangeError('size', size);
}
} |
| // Pass the message to the constructor instead of setting it on the object | ||
| // to make sure it is the same as the one created in C++ | ||
| // eslint-disable-next-line no-restricted-syntax | ||
| const err = new Error(message); | ||
| Error.stackTraceLimit = tmpLimit; |
There was a problem hiding this comment.
This seems like something V8 folks should be made aware of since, in this case, the lazily populated .stack isn't even being accessed yet. The fact that setting the Error.stackTraceLimit to 0 appears to improves basic error creation performance when the .stack isn't accessed is counterintuitive at least.
I'd rather this be tracked as a V8 issue and fixed there than throwing Error.stackTraceLimit = 0 all over the Node codebase.
There was a problem hiding this comment.
As mentioned above, I already did that :)
There was a problem hiding this comment.
Yes, but the V8 issues mentioned above
- 8451 – loading Error.stackTraceLimit could be done faster:
We load Error.stackTraceLimit every time we collect a stack trace. Maybe this is something we want to improve? See https://docs.google.com/document/d/1wwigvXVQ-mdLmSfNnLie2xVQGCt-6AKhP1OuyE38Vw8/edit
- 8452 – Faster stack traces:
Stack traces are not used a lot in benchmarks, but often on real web sites for telemetry purposes. Some Node.js frameworks also use it to implement async stack traces. We could use better performance for stack trace collection in general
don't clearly state this specific case though.
There was a problem hiding this comment.
I opened another issue to properly address this: https://bugs.chromium.org/p/v8/issues/detail?id=8555
|
I also doubt that stack traces will be fast soon. As soon as that's done, removing these should not be an issue. I do not have a strong opinion about this PR as it's only about error cases but I don't think that adding this hurts in any way. What I could try is to implement my suggestion (#24747 (comment)) instead as it's cleaner code, if requested. |
|
Just in case it got lost in the replies, my comment #24747 (comment) has not been addressed. |
Can confirm. We have plans, but won't be able to do any time soon. |
|
Ping @jdalton |
BridgeAR
added
the
author ready
|
Given the failures of test-cli-syntax on multiple hosts, I'm going to do a Rebuild rather than a Resume CI so it rebases and gets a recent commit that hopefully makes that failure less frequent: |
|
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19340/ ✔️ |
|
Landed in a1a5c04 |
In case of an error where we only care about a cleaned up stack trace it is cheaper to reset the stack trace limit for the error that is created. That way the stack frames do not have to be computed twice. PR-URL: nodejs#24747 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
In case of an error where we only care about a cleaned up stack trace it is cheaper to reset the stack trace limit for the error that is created. That way the stack frames do not have to be computed twice. PR-URL: #24747 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
BridgeAR
added
the
performance
In case of an error where we only care about a cleaned up stack trace it is cheaper to reset the stack trace limit for the error that is created. That way the stack frames do not have to be computed twice. PR-URL: nodejs#24747 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
In case of an error where we only care about a cleaned up stack
trace it is cheaper to reset the stack trace limit for the error
that is created. That way the stack frames do not have to be
computed twice.
In a local mini benchmark it reduced the Error creation time by around
30 %.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes