Closed Bug 1398751 Opened 7 years ago Closed 7 years ago

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


(Core :: JavaScript Engine, enhancement)

Not set



Tracking Status
firefox57 --- fixed


(Reporter: anba, Assigned: anba)





(2 files, 2 obsolete files)

This will lead to almost exactly the same speed-ups as in V8 (, i.e. 2-3x faster on the µ-benchmark in
This improves the benchmark in (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)
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]

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]

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+
(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
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]

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

Attachment #8906747 - Flags: review?(evilpies) → review+
Invert condition in if-statement as suggested in review comment. Carrying r+ from :evilpie.
Attachment #8906607 - Attachment is obsolete: true
Attachment #8907277 - Flags: review+
Pushed by
Part 1: Add fast-path for typed arrays in js::GetElements to speed-up Function.apply with typed arrays. r=evilpie
Part 2: Add fast path to convert int32 value to a code point. r=evilpie
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.