perf_hooks: fix rangeerror #54772
perf_hooks: fix rangeerror #54772
Conversation
|
Review requested:
|
| @@ -106,6 +106,7 @@ performance of code in Node.js. | |||
|
|
|||
| * 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
|
As a test, I think it makes sense to use the code from the issue. |
| @@ -6,7 +6,6 @@ const { | |||
| 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 (let i = 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
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)