If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla37

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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".
(Assignee)

Comment 1

3 years ago
Tentative fix: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c1b56c7c228e
(Assignee)

Comment 2

3 years ago
Created attachment 8532137 [details] [diff] [review]
bug1107365-rename.diff

Prep work: Rename 'AnyTypedArray' in some contexts to avoid confusion with the other meaning of 'AnyTypedArray'.
Attachment #8532137 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

3 years ago
Created attachment 8532139 [details] [diff] [review]
bug1107365-generalize.diff

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]
bug1107365-rename.diff

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]
bug1107365-generalize.diff

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.
(Assignee)

Comment 7

3 years ago
With review comments applied:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/389db774250d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1566e6f3d0

+1 on self-hosting, with accompanying benchmarks.
https://hg.mozilla.org/mozilla-central/rev/389db774250d
https://hg.mozilla.org/mozilla-central/rev/4b1566e6f3d0
Status: NEW → RESOLVED
Last Resolved: 3 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.