Add a fast path for typed arrays to js::GetElements

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
This will lead to almost exactly the same speed-ups as in V8 (https://bugs.chromium.org/p/v8/issues/detail?id=2435), i.e. 2-3x faster on the µ-benchmark in https://bugs.chromium.org/p/v8/issues/detail?id=2435#c6.
(Assignee)

Comment 1

2 years ago
This improves the benchmark in https://bugs.chromium.org/p/v8/issues/detail?id=2435#c6 (with |n| changed to 1e4 instead of 1e5, because my PC is slower and I'm testing in a VM :-) ) as follows:

stringFromCharCode: 985ms -> 305ms
stringFromCodePoint: 1210ms -> 515ms

Before the V8 patch, V8 and SM were both slower than JSC/Chakra, now that V8 is patched, we're the slowest engine for that µ-benchmark. But with this patch, we're faster than JSC/Chakra and I guess we get comparable numbers to V8 again, but since I haven't yet recompiled V8, I can't tell for sure.
Attachment #8906606 - Flags: review?(evilpies)
(Assignee)

Comment 2

2 years ago
And this gives a further improvement for the stringFromCodePoint case:

With patch 1:   1210ms -> 515ms
With patch 1+2: 1210ms -> 335ms
Attachment #8906607 - Flags: review?(evilpies)
Comment on attachment 8906606 [details] [diff] [review]
bug1398751-part1-getelements-for-typedarrays.patch

Review of attachment 8906606 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, very clean.

::: js/src/jsarray.cpp
@@ +443,5 @@
> +    MOZ_ASSERT(length == typedArray->length());
> +    MOZ_ASSERT_IF(length > 0, !typedArray->hasDetachedBuffer());
> +
> +    for (uint32_t i = 0; i < length; ++i, ++vp) {
> +        *vp = typedArray->getElement(i);

I hope compilers are clever enough to do loop unswitching here and we don't check the type in every iteration.
Attachment #8906606 - Flags: review?(evilpies) → review+
Comment on attachment 8906607 [details] [diff] [review]
bug1398751-part2-codepoint-from-int32.patch

Review of attachment 8906607 [details] [diff] [review]:
-----------------------------------------------------------------

Wow this is a big difference.

::: js/src/jsstr.cpp
@@ +3315,5 @@
> +
> +    // Fast path for the common case - the input is already an int32.
> +    if (code.isInt32()) {
> +        int32_t nextCP = code.toInt32();
> +        if (!(nextCP < 0 || nextCP > int32_t(unicode::NonBMPMax))) {

I think our usual style is to invert the conditions.
Attachment #8906607 - Flags: review?(evilpies) → review+
(Assignee)

Comment 5

2 years ago
(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #3)
> I hope compilers are clever enough to do loop unswitching here and we don't
> check the type in every iteration.

Good remark! I just tested this and at least GCC 5.4 doesn't seem to hoist the type check out of the loop. Manually switching the loop and the switch-statement further improved the µ-benchmark to

stringFromCharCode: 215ms
stringFromCodePoint: 240ms
(Assignee)

Comment 6

2 years ago
New patch for part 1, because compilers don't seem to perform loop unswitching. Reduces the time needed to complete the benchmark by another 90ms when compared to the previous patch.
Attachment #8906606 - Attachment is obsolete: true
Attachment #8906747 - Flags: review?(evilpies)
Comment on attachment 8906747 [details] [diff] [review]
bug1398751-part1-new-getelements-for-typedarrays.patch

Review of attachment 8906747 [details] [diff] [review]:
-----------------------------------------------------------------

Great
Attachment #8906747 - Flags: review?(evilpies) → review+
(Assignee)

Comment 8

2 years ago
Invert condition in if-statement as suggested in review comment. Carrying r+ from :evilpie.
Attachment #8906607 - Attachment is obsolete: true
Attachment #8907277 - Flags: review+

Comment 10

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbf2ccf813e
Part 1: Add fast-path for typed arrays in js::GetElements to speed-up Function.apply with typed arrays. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f98cfb1a45
Part 2: Add fast path to convert int32 value to a code point. r=evilpie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fdbf2ccf813e
https://hg.mozilla.org/mozilla-central/rev/f0f98cfb1a45
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Updated

10 months ago
Duplicate of this bug: 885521
You need to log in before you can comment on or make changes to this bug.