perf_hooks: fix rangeerror#54772
perf_hooks: fix rangeerror #54772
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Sep 4, 2024
Review requested:
|
| * Methods that mutate the internal state of arrays: | ||
| *`ArrayPrototypePush` | ||
| *`ArrayPrototypePushApply`: also fails with a RangeError on large arrays |
There was a problem hiding this comment.
That’s not the correct place to document, as this is not a performance issue IIUC
aduh95 commented Sep 4, 2024
As a test, I think it makes sense to use the code from the issue. |
| ArrayPrototypeFilter, | ||
| ArrayPrototypeIncludes, | ||
| ArrayPrototypePush, | ||
| ArrayPrototypePushApply, |
There was a problem hiding this comment.
Unrelated to this PR: Can we eventually add an eslint rule to avoid using this?
There was a problem hiding this comment.
What would be the rational for such a rule?
There was a problem hiding this comment.
V8 has a hard limit on function argument count and without explicitly checking the number, it is easy to make a mistake?
There was a problem hiding this comment.
Depending on use it can also be significantly faster. In undici we batch the .apply calls in our data url parser for performance: https://github.com/nodejs/undici/blob/89a46dd54f7d7db7513c435fac8042769ee9e9b5/lib/web/fetch/data-url.js#L657-L674
| ArrayPrototypePushApply(this.#buffer,entries); | ||
| for(leti=0;i<entries.length;i++){ | ||
| ArrayPrototypePush(this.#buffer,entries[i]); | ||
| } |
There was a problem hiding this comment.
I understand the necessary fix but we should benchmark this to make sure it's not too much of a regression. Might make sense to split entries up and still use ...PushApply with smaller, safer chunks
avivkeller added the perf_hooks 
github-actionsbot removed the request-ci nodejs-github-bot commented Oct 15, 2024
marco-ippolito added commit-queue 
marco-ippolito commented Nov 2, 2024
Actually this is technically landable but there are some unresolved conversation |
aduh95 commented Oct 13, 2025
Superseded by #60084 |
9 participants
Fixes: #54768
A proposed ad-hoc fix for that issue.
As mentioned there I'm not sure how to test since it's implementation detail #54768 (comment)