Account for resize-in-the-middle in TA methods #74
Conversation
Closes #73 At a high-level: - Lengths are cached at the beginning of methods. - Any element copy where all bytes of the element are within bounds are, the copy is performed with GetValueFromBuffer and SetValueInBuffer to preserve byte-level encoding. - If the length changed due to a resize-in-the-middle, then an element copy can be partially out of bounds either in source or in target. In that case, the copy is done with Get and Set.
|
@marjakh It might be easier to review this by downloading the fork and viewing |
| 1. If _count_ > 0, then | ||
| 1. NOTE: The copying must be performed in a manner that preserves the bit-level encoding of the source data. | ||
| 1. Let _buffer_ be _O_.[[ViewedArrayBuffer]]. | ||
| 1. <ins>Let _regetBufferByteLength_ to MakeIdempotentArrayBufferByteLengthGetter(~SeqCst~).</ins> |
There was a problem hiding this comment.
Small typo here and in "slice":
-1. <ins>Let _regetBufferByteLength_ to MakeIdempotentArrayBufferByteLengthGetter(~SeqCst~).</ins>
+1. <ins>Let _regetBufferByteLength_ be MakeIdempotentArrayBufferByteLengthGetter(~SeqCst~).</ins>...though I wonder what you think about re-assigning "getBufferByteLength" instead. While a distinct variable highlights the consideration we're making, it also insinuates that there's further need for the original closure. As I understand it, we do not need or want the original value of "getBufferByteLength" at this point, so over-writing it would be more clear from that perspective. We could use an informative note to underscore the consideration and also explain it a bit.
There was a problem hiding this comment.
It is a bit confusing, but the original closure could still be used. The reason this idempotent getter exists is to avoid multiple shared memory loads of the length, because each such load is always observable. So the getter loads it once and caches it. Once cached, you could re-use the getter. It's... not a great solution, and probably warrants a bigger refactor when we're getting ready for Stage 4. I'd like to discuss with the other editors what's a clearer way here.
There was a problem hiding this comment.
My suggestion is editorially-motivated. While the original closure could still be used, I can't think of a case where the cached value would be pertinent. By storing the new closure in a new variable, the text suggests that such a case exists, and it presents a hazard for future editors who may mistakenly use "getBufferByteLength" instead of "regetBufferByteLength".
There was a problem hiding this comment.
Fair enough, but I think the bigger problem is that the idempotent getter thing is just not a very clear mechanism to begin with. Happy to change it here to setting.
There was a problem hiding this comment.
Yeah, if you expect this will be refactored, then I agree that this isn't so important. Though we still ought to correct the grammar ("to" to "be").
|
(Now I actually ended up reading every line :) ) |
|
Q: don't we need to reload the length also in ArrayBuffer.prototype.slice? |
This PR does that in step 16.i, here in the source. Did I miss it somewhere else? |
| @@ -596,7 +671,9 @@ <h1>%TypedArray%.prototype.slice ( _start_, _end_ )</h1> | |||
| <emu-alg> | |||
| 1. Let _O_ be the *this* value. | |||
| 1. Perform ? ValidateTypedArray(_O_). | |||
| 1. Let _len_ be _O_.[[ArrayLength]]. | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
That line is in %TypedArray%.prototype.slice, no? I'm talking about ArrayBuffer.prototype.slice. |
|
Quite right, I missed the AB and SAB methods. Will fix. |
Closes #73 At a high-level: - Lengths are cached at the beginning of methods. - Only in-bounds elements are copied. Together, this means that the result TAs have the snapshotted length at the beginning of the method. If the source TA's buffer shrunk due to the species constructor, the elements between [newShrunkLen, originalLen) in the result TA remain 0. This is an alternative solution to #74. In #74, elements between [newShrunkLen, originalLen) are set to the result of OOB [[Get]]s on the source TA. While doing so is maximally consistent with the Array versions of these methods, it turns out to be really weird for floating-point TAs. For OOB [[Get]]s on TAs returns `undefined`, and setting `undefined` in Float32Arrays and Float64Arrays coerces to NaN. This means extra complexity and code implementations have to do for an edge case (resize-in-the-middle) that's bad practice anyway.
|
Closing this in favor of #75 |
Closes #73 At a high-level: - Lengths are cached at the beginning of methods. - Only in-bounds elements are copied. Together, this means that the result TAs have the snapshotted length at the beginning of the method. If the source TA's buffer shrunk due to the species constructor, the elements between [newShrunkLen, originalLen) in the result TA remain 0. This is an alternative solution to #74. In #74, elements between [newShrunkLen, originalLen) are set to the result of OOB [[Get]]s on the source TA. While doing so is maximally consistent with the Array versions of these methods, it turns out to be really weird for floating-point TAs. For OOB [[Get]]s on TAs returns `undefined`, and setting `undefined` in Float32Arrays and Float64Arrays coerces to NaN. This means extra complexity and code implementations have to do for an edge case (resize-in-the-middle) that's bad practice anyway.
Closes #73 At a high-level: - Lengths are cached at the beginning of methods. - Only in-bounds elements are copied. Together, this means that the result TAs have the snapshotted length at the beginning of the method. If the source TA's buffer shrunk due to the species constructor, the elements between [newShrunkLen, originalLen) in the result TA remain 0. This is an alternative solution to #74. In #74, elements between [newShrunkLen, originalLen) are set to the result of OOB [[Get]]s on the source TA. While doing so is maximally consistent with the Array versions of these methods, it turns out to be really weird for floating-point TAs. For OOB [[Get]]s on TAs returns `undefined`, and setting `undefined` in Float32Arrays and Float64Arrays coerces to NaN. This means extra complexity and code implementations have to do for an edge case (resize-in-the-middle) that's bad practice anyway.
Closes #73
At a high-level:
the copy is performed with GetValueFromBuffer and SetValueInBuffer to
preserve byte-level encoding.
copy can be partially out of bounds either in source or in target. In
that case, the copy is done with Get and Set.