GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — errors: improve hideStackFrames by Uzlopak · Pull Request #49990 · nodejs/node Conversation
|
|
Review requested:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
what's the benefit/performance diff? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
My benchmarks dont show regressions. The benefit is, that the stack trace will be generated propery, without the need to call captureLargerStackTrace. So we wont be forced to keep infinity amount of CallSites just to kick some. On the long run, i want to try to remove prepareStackTrace. Currently i investigate what this overrideStackTrace Map is doing, and how we can avoid it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Won't this mess with the .name of the error and how stack traces look (vs the previous implementation)?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The only reason i created this class, was because to avoid to patch everywhere the logic for Error.stackTraceLimit over and over.
The name should be the same imho. But i can tackle it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thanks let me know when ready
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Does this test cover the issue?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
cd3b2c9 to
a12adde
Compare
|
The existing tests pass. Next step is to remove the captureLargerStackTrace function and ensure that the logic is equivalent. Imho this is an exciting PR for me. If done right, this should improve the performance of basically every error case. And tbh it seems to be a smart solution (patting on my own shoulder). Also it will cover all the validateX functions.so it should expose less unnecessary stacks. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
note to myself: Investigate https://docs.google.com/document/d/13Sy_kBIJGP0XT34V1CV3nkWya4TwYx9L3Yv45LdGB6Q/edit#heading=h.9ss45aibqpw2 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
a2134fc to
3f94be4
Compare
|
@anonrig can you please add the author-ready tag? I would like to discuss this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Also @BridgeAR |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
da8ff55 to
134ea2b
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
You are correct |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| }); | ||
| if (otherClasses.includes(HideStackFramesError)) { | ||
| if (otherClasses.length !== 1) { | ||
| otherClasses.forEach((clazz) => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| otherClasses.forEach((clazz) => { | ||
| def[clazz.name] = makeNodeErrorWithCode(clazz, sym); | ||
| }); | ||
| if (otherClasses.includes(HideStackFramesError)) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I would recommend we do.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@benjamingr |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Don't have capacity, sorry, unblocking so others can progress
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@anonrig |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@anonrig |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/55419/ 💛 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed in 83e6350 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: #49990 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #49990 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #49990 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: James M Snell <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.