Closed Bug 1107365 Opened 5 years ago Closed 5 years ago

TypedArray.prototype.set must take the fast path when copying from SharedTypedArray, and vice versa


(Core :: JavaScript Engine, defect)

Not set





(Reporter: lth, Assigned: lth)


(Blocks 1 open bug)



(2 files)

Right now the copy ends up taking the most expensive path (setFromNonTypedArray) because the types are not recognized as being compatible.  This problem is possibly caused by a confusion: the template parameter name for much of the typed array code is AnyTypedArray, but elsewhere in the code this identifier is used to identify "TypedArray or SharedTypedArray".
Prep work: Rename 'AnyTypedArray' in some contexts to avoid confusion with the other meaning of 'AnyTypedArray'.
Attachment #8532137 - Flags: review?(jwalden+bmo)
Generalize the set() method to handle the case where one type is a TypedArray and the other is a SharedTypedArray, and vice versa.
Attachment #8532139 - Flags: review?(jwalden+bmo)
Comment on attachment 8532137 [details] [diff] [review]

Review of attachment 8532137 [details] [diff] [review]:

"but elsewhere in the code this identifier is used to identify 'TypedArray or SharedTypedArray'"  Bah!  *grmbls*
Attachment #8532137 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8532139 [details] [diff] [review]

Review of attachment 8532139 [details] [diff] [review]:

::: js/src/vm/TypedArrayCommon.h
@@ +706,5 @@
>          MOZ_ASSERT(len <= target->length() - offset);
>          if (source->is<SomeTypedArray>()) {
>              Rooted<SomeTypedArray*> src(cx, &source->as<SomeTypedArray>());
> +            return setFromAnyTypedArray(cx, target, src, offset);

You can just pass |source| here without the extra rooting/typing, right?  And can't the test be IsAnyTypedArray, too?  Guessing right now this still doesn't really work with shared typed arrays as source.

@@ +720,1 @@
>      {

Add a MOZ_ASSERT(IsAnyTypedArray(source), "use setFromNonTypedArray instead"); please.  (Might be this is trivial because only one caller, but this is defending against the future mostly.)
Attachment #8532139 - Flags: review?(jwalden+bmo) → review+
For what it's worth, the problem wasn't "possibly caused by a confusion" -- just caused by not thinking about the cross-type copying case.

But really, we need to self-host all this ASAP so we're not relying on C++ to do any of the accesses that are supposed to be atomic.  We have the self-hosting intrinsics, so I should just get on this soon.
With review comments applied:


+1 on self-hosting, with accompanying benchmarks.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1199578
You need to log in before you can comment on or make changes to this bug.