Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array.fill and TypedArray.fill polyfills #3103

Merged
merged 2 commits into from
Apr 20, 2021
Merged

Conversation

mvaligursky
Copy link
Contributor

We got burnt on this being missing few times, and it likely gives us enough performance benefit to have the polyfill vs using the loop to work around it.


// implementation for typed arrays
if (!Int8Array.prototype.fill) {
Int8Array.prototype.fill = Array.prototype.fill;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this work correctly if Array.fill implementation was provided (not polyfilled)? might be worth splitting out the function and using it for all polyfills (including Array).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not expect a problem with this? But we only have IE to test on and that is missing both. I tested on IE and this works, and is suggested polyfill (see links I added)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your test worked.

However this code suggests it's possible that Array.fill is provided by the browser while Int8Array.fill isn't. Under such a circumstance this code very well may fail.

It is simple enough to make this code not fail and I don't really understand why you don't do it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, currently a bit misleading because I would expect array-fill.js to just polyfill Array#fill. I'd put the rest in typedarray-fill.js.

@willeastcott willeastcott self-requested a review April 20, 2021 10:03
@mvaligursky mvaligursky merged commit d96ba3e into master Apr 20, 2021
@mvaligursky mvaligursky deleted the mvaligursky-array-fill branch April 20, 2021 10:08
@Maksims
Copy link
Contributor

Maksims commented Apr 20, 2021

I do understand this polyfill comes from MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill

But it definitely is a bit too verbose, with anti-patterns used:

  1. Using of arguments leads to function deoptimisations on V8 and other engines. It is best to pre-define arguments in a function, and use them.
  2. Object(o) - leads to allocations?

@mvaligursky
Copy link
Contributor Author

@Maksims that's true, but this is just a polyfill for IE, not a generic code executing on all browsers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants