/ecma262

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

CloneArrayBuffer copies the underlying ArrayBuffer until the end, which callers don't need #447

Closed
littledan opened this Issue Mar 1, 2016 · 6 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
4 participants
@littledan @leobalter @allenwb @bterlson
@littledan
Member

littledan commented Mar 1, 2016

The TypedArray constructor includes a fast path for a TypedArray input (as opposed to treating it as an iterable), which has a fast path for the case of the same-type input and output. This calls CloneArrayBuffer. CloneArrayBuffer takes an argument for the start offset, but not for the end, so it copies the underlying ArrayBuffer until the end. Then, the caller sets the [[ArrayLength]] appropriately so that it gets the right length array. However, the underlying ArrayBuffer is still accessible through get %TypedArray%.prototype.buffer, so the extra unnecessary allocation is observable. Would it make sense to change the TypedArray constructor definition to not allocate and keep around this extra memory?

@bterlson bterlson added the question label Mar 3, 2016

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 4, 2016

Member

@allenwb @kmiller68 @leobalter What do you think about this issue?
Member

littledan commented Mar 4, 2016

@allenwb @kmiller68 @leobalter What do you think about this issue?

littledan added a commit to littledan/ecma262 that referenced this issue Mar 4, 2016

@littledan
In the TypedArray(typedArray) constructor, copy only the needed portion 
The TypedArray constructor includes a "fast path" for a TypedArray input (as
opposed to treating it as an iterable), which has a fast path for the case of
the same-type input and output. Previously, this called CloneArrayBuffer.
CloneArrayBuffer takes an argument for the start offset, but not for the end,
so it copied the underlying ArrayBuffer until the end. The extra wasted copy
remains observable, e.g. in cases like

new Uint8Array(new Uint8Array(10000).subarray(0, 1)).buffer.byteLength // => 1 or 10000 ?

This patch refactors CloneArrayBuffer into CloneUnderlyingArrayBuffer which
clones only the viewed part of the ArrayBuffer, taking the end into account.
This does not make an observable change for the other caller
(%TypedArray%.prototype.set), where it would have already been valid to apply
this optimization.

Closes tc39#447
8a60672

@littledan littledan referenced this issue Mar 4, 2016

Merged

In the TypedArray(typedArray) constructor, copy only the needed portion #458

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Mar 4, 2016

Member

Would it make sense to change the TypedArray constructor definition to not allocate and keep around this extra memory?

Your commit (8a60672) seems interesting and helps looking through this issue.

Currently, it seems the new clone is limited to the expected src buffer bytelength as in steps 8 and 11 on CloneArrayBuffer, eliminating the excess from the offset.

8. Let targetBuffer be ? AllocateArrayBuffer(cloneConstructor, cloneLength).

On your changes, you also limit these changes to the bytelength of the typedArray, eliminating the remaining unused buffer bytelength after the typedArray bytelength is considered.

That seems reasonable and indeed might avoid extra memory use, especially in case you have a subarray sample.
Member

leobalter commented Mar 4, 2016

Would it make sense to change the TypedArray constructor definition to not allocate and keep around this extra memory?

Your commit (8a60672) seems interesting and helps looking through this issue.

Currently, it seems the new clone is limited to the expected src buffer bytelength as in steps 8 and 11 on CloneArrayBuffer, eliminating the excess from the offset.

8. Let targetBuffer be ? AllocateArrayBuffer(cloneConstructor, cloneLength).

On your changes, you also limit these changes to the bytelength of the typedArray, eliminating the remaining unused buffer bytelength after the typedArray bytelength is considered.

That seems reasonable and indeed might avoid extra memory use, especially in case you have a subarray sample.
@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 4, 2016

Member

@leobalter Exactly. To clarify from the spec text, see the way that cloneLength is calculated:

6 . Let cloneLength be srcLength - srcByteOffset.

The early end is not taken into account, only the offset.
Member

littledan commented Mar 4, 2016

@leobalter Exactly. To clarify from the spec text, see the way that cloneLength is calculated:

6 . Let cloneLength be srcLength - srcByteOffset.

The early end is not taken into account, only the offset.
@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Mar 4, 2016

Member

I agree, this is a bug. The fast path should not produce a buffer with extra space.

Personally, I'd fix it by adding a cloneLength parameter to CloneArrayBuffer
Member

allenwb commented Mar 4, 2016

I agree, this is a bug. The fast path should not produce a buffer with extra space.

Personally, I'd fix it by adding a cloneLength parameter to CloneArrayBuffer
@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 8, 2016

Member

@allenwb Good suggestion, especially seeing how @annevk pointed out HTML uses this operation in a way which would be unusable with the new definition. I wish we had multiple named optional arguments in ECMASpeak! I'll modify this patch for both of these concerns.
Member

littledan commented Mar 8, 2016

@allenwb Good suggestion, especially seeing how @annevk pointed out HTML uses this operation in a way which would be unusable with the new definition. I wish we had multiple named optional arguments in ECMASpeak! I'll modify this patch for both of these concerns.

littledan added a commit to littledan/ecma262 that referenced this issue Mar 9, 2016

@littledan
In the TypedArray(typedArray) constructor, copy only the needed portion 
The TypedArray constructor includes a "fast path" for a TypedArray input (as
opposed to treating it as an iterable), which has a fast path for the case of
the same-type input and output. Previously, this called CloneArrayBuffer.
CloneArrayBuffer takes an argument for the start offset, but not for the end,
so it copied the underlying ArrayBuffer until the end. The extra wasted copy
remains observable, e.g. in cases like

new Uint8Array(new Uint8Array(10000).subarray(0, 1)).buffer.byteLength // => 1 or 10000 ?

This patch adds an additional argument to CloneArrayBuffer for the length
in bytes that should be cloned, so that only the viewed portion of the
ArrayBuffer is copied.
This does not make an observable change for the other caller
(%TypedArray%.prototype.set), where it would have already been valid to apply
this optimization.

Closes tc39#447
3a09932
@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 9, 2016

Member

The new version is intended to address these concerns. Any thoughts? Thanks for the feedback.
Member

littledan commented Mar 9, 2016

The new version is intended to address these concerns. Any thoughts? Thanks for the feedback.

littledan added a commit to littledan/ecma262 that referenced this issue Mar 9, 2016

@littledan
In the TypedArray(typedArray) constructor, copy only the needed portion 
The TypedArray constructor includes a "fast path" for a TypedArray input (as
opposed to treating it as an iterable), which has a fast path for the case of
the same-type input and output. Previously, this called CloneArrayBuffer.
CloneArrayBuffer takes an argument for the start offset, but not for the end,
so it copied the underlying ArrayBuffer until the end. The extra wasted copy
remains observable, e.g. in cases like

new Uint8Array(new Uint8Array(10000).subarray(0, 1)).buffer.byteLength // => 1 or 10000 ?

This patch adds an additional argument to CloneArrayBuffer for the length
in bytes that should be cloned, so that only the viewed portion of the
ArrayBuffer is copied.
This does not make an observable change for the other caller
(%TypedArray%.prototype.set), where it would have already been valid to apply
this optimization.

Closes tc39#447
ab69566

littledan added a commit to littledan/ecma262 that referenced this issue Mar 25, 2016

@littledan
In the TypedArray(typedArray) constructor, copy only the needed portion 
The TypedArray constructor includes a "fast path" for a TypedArray input (as
opposed to treating it as an iterable), which has a fast path for the case of
the same-type input and output. This path calls CloneArrayBuffer.
CloneArrayBuffer takes an argument for the start offset, but it did not take
an argument for the end, so it copied the underlying ArrayBuffer until the
end. The extra wasted copy remains observable, e.g. in cases like

new Uint8Array(new Uint8Array(10000).subarray(0, 1)).buffer.byteLength // => 1 or 10000 ?

This patch adds an additional argument to CloneArrayBuffer for the length
in bytes that should be cloned, so that only the viewed portion of the
ArrayBuffer is copied.
This does not make an observable change for the other caller
(%TypedArray%.prototype.set), where it would have already been valid to apply
this optimization.

Closes tc39#447
ff2d343

@bterlson bterlson closed this in #458 Apr 18, 2016

bterlson added a commit that referenced this issue Apr 18, 2016

@littledan @bterlson
Normative: In the TypedArray(typedArray) constructor, copy only the n… 
…eeded portion (#458)

The TypedArray constructor includes a "fast path" for a TypedArray input (as
opposed to treating it as an iterable), which has a fast path for the case of
the same-type input and output. This path calls CloneArrayBuffer.
CloneArrayBuffer takes an argument for the start offset, but it did not take
an argument for the end, so it copied the underlying ArrayBuffer until the
end. The extra wasted copy remains observable, e.g. in cases like

new Uint8Array(new Uint8Array(10000).subarray(0, 1)).buffer.byteLength // => 1 or 10000 ?

This patch adds an additional argument to CloneArrayBuffer for the length
in bytes that should be cloned, so that only the viewed portion of the
ArrayBuffer is copied.
This does not make an observable change for the other caller
(%TypedArray%.prototype.set), where it would have already been valid to apply
this optimization.

Closes #447
e1cecfa
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment