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

feat: add functional equivalent of Array.prototype.copyWithin and TypedArray.prototype.copyWithin #367

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ShraddheyaS
Copy link
Contributor

Resolves #203.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

  • Read, understood, and followed the contributing guidelines, including the relevant style guides.
  • Read and understand the Code of Conduct.
  • Read and understood the licensing terms.
  • Searched for existing issues and pull requests before submitting this pull request.
  • Filed an issue (or an issue already existed) prior to submitting this pull request.
  • Rebased onto latest develop.
  • Submitted against develop branch.

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.


@stdlib-js/reviewers

@kgryte
Copy link
Member

kgryte commented Sep 28, 2020

@ShraddheyaS Thanks for the PR! copyWithin is actually more interesting than I anticipated. Notably, the algo you used (akin to the MDN polyfill implementation) is good for the generic case where you need to account for sparse array-like objects. However, we can do more interesting things with Typed Arrays. Notably, for, e.g., Uint8Array inputs, we could copy in 4 byte increments (i.e., interpret the underlying array buffer as a Uint32Array) plus any remaining bytes. In C, we could even interpret as double, I believe, but we cannot do that in JavaScript due to canonicalized NaNs.

Also, did you include in your test cases whether the polyfill properly accounts for overlapping buffers? Not clear to me, from the algo, whether this is true. I would expect in those scenarios that we’d need to copy-and-paste, but maybe I am missing something from the algo.

@ShraddheyaS
Copy link
Contributor Author

@kgryte Yes, that does sound more performant instead of copying individual byte-sized elements for the Uint8Array inputs.

By overlapping buffers, do you mean the case where the target is after start in which the portion to be copied is going to be overwritten at the end of the operation? If yes, then there is a test case for that scenario. The algorithm handles that by copying elements in the reverse direction (steps 16-17 in the MDN polyfill).

count = min( final-from, len-to );
direction = 1;
if ( from < to && to < ( from+count ) ) {
	direction = -1;
	from += count - 1;
	to += count - 1;
}

Processing Uint8Array inputs differently would mean handling the case of overlapping buffers in a slightly different way around the target bytes in above.

@kgryte
Copy link
Member

kgryte commented Oct 1, 2020

@ShraddheyaS Re: overlapping buffers. Thanks for the clarification.

Re: algo. Yeah, I am not sure, atm, whether the potential perf gains would pan out, but seems like something we may want to investigate.

Regardless, I think we may want to treat typed arrays differently than generic arrays and array-like objects. Notably, the from in collection is not generally applicable to typed arrays, as they are contiguous blocks of memory. We don't need to perform a look-up for the property before copying. This is mainly applicable for sparse arrays.

Presumably, we could also treat generic arrays as being dense. Sparse generic arrays are not particularly common, imo, and have perf cliffs. We may only want to apply sparse treatment to array-like objects. This would, however, cause us to differ from the spec, which I am fine with, so long as we have reasonable justification.

@Planeshifter Planeshifter changed the title Add functional equivalent of Array.prototype.copyWithin and TypedArray.prototype.copyWithin feat: add functional equivalent of Array.prototype.copyWithin and TypedArray.prototype.copyWithin Sep 8, 2024
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.

RFC: add functional equivalent of Array.prototype.copyWithin
2 participants