Improve StringIterator performance

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

It shouldn't be too hard to optimize StringIterator performance by adapting evilpie's changes from bug 1352006 and bug 1353170.
Definitely, we can probably share some of the code as well.
Blocks: es6perf
Renames all (L|M|R)NewArrayIterator types/methods to (L|M|R)NewIterator and adds a MNewIterator::Type enum, so we can add a StringIterator type in part 2.
Attachment #8870476 - Flags: review?(evilpies)
Adds the necessary bits to inline StringIterator in Ion. This was fairly easy because everything was already there from your ArrayIterator patches. 

The new recover-newstringiterator.js test case was simply copied from the existing recover-newarrayiterator.js, I hope that's sufficient for testing the recover instructions.

Part 3 will revert the js/src/builtin/String.js changes and instead use String.fromCodePoint to create the result string. So the changes are simply present to test that the StringIterator can be inlined or in case part 3 gets rejected.
Attachment #8870480 - Flags: review?(evilpies)
Allow to inline String.fromCodePoint when called with non-Latin1 code points. 

Calling String.prototype.substring was still kind of slow, so I decided to extend the code-gen for String.fromCodePoint to handle all code points.

Because I still know far too little about the code generator, this is the part which probably requires most of your attention. For example, I don't know if I've chosen the correct masm methods or this approach is the most efficient one. You have been warned! :-)


Improves this µ-benchmark from 430 ms to 220 ms for me:

    var s = "\u{100}\u{100}\u{100}\u{100}\u{100}";
    var q = 0;
    var t = Date.now();
    for (var i = 0; i < 1000000; ++i) {
        for (var c of s) q += c.codePointAt(0);
    }
    print(Date.now()-t, q);
Attachment #8870483 - Flags: review?(evilpies)
Comment on attachment 8870476 [details] [diff] [review]
bug1367088-part1-generalize-newarrayiterator-codegen.patch

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

Very nice
Attachment #8870476 - Flags: review?(evilpies) → review+
Attachment #8870480 - Flags: review?(evilpies) → review+
Comment on attachment 8870483 [details] [diff] [review]
bug1367088-part3-inline-fromcodepoint-non-latin1.patch

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

Looks very nice.

::: js/src/jit/CodeGenerator.cpp
@@ +8061,5 @@
>  
>      // Use a bailout if the input is not a valid code point, because
>      // MFromCodePoint is movable and it'd be observable when a moved
>      // fromCodePoint throws an exception before its actual call site.
>      bailoutCmp32(Assembler::Above, codePoint, Imm32(unicode::NonBMPMax), snapshot);

Nit: Move this into the isTwoByte block now.

@@ +8099,5 @@
> +            masm.computeEffectiveAddress(Address(output, JSInlineString::offsetOfInlineStorage()),
> +                                         temp1);
> +
> +            masm.store16(codePoint, Address(temp1, 0));
> +            masm.addPtr(Imm32(sizeof(char16_t)), temp1);

I would just do masm.store16(Imm32(0), Address(temp1, sizeof(char16_t))) instead of adding the pointer and doing it later.

@@ +8125,5 @@
> +            masm.move32(codePoint, temp2);
> +            masm.and32(Imm32(0x3FF), temp2);
> +            masm.or32(Imm32(unicode::TrailSurrogateMin), temp2);
> +
> +            masm.store16(temp2, Address(temp1, 0));

Again can use Address(temp1, sizeof(char16_t))
Attachment #8870483 - Flags: review?(evilpies) → review+
Comment on attachment 8870476 [details] [diff] [review]
bug1367088-part1-generalize-newarrayiterator-codegen.patch

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

::: js/src/jit/Recover.h
@@ +599,3 @@
>  {
> +  private:
> +    MNewIterator::Type type_;

nit: Do not use MIR type in this file!
MIR.h should not be included by Recover.h for compilation performance reasons. (*)

Use a uint8_t as storage, and coerce it back in the recover method implemented in Recover.cpp, which can include MIR.h.

I will open a follow-up bug to remove the existing include.

(*) jit/MIR.h is the biggest header of SpiderMonkey, and jit/Recover.h is needed for iterating the stack, thus included by vm/Stack.h which is included in almost everything.
(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> nit: Do not use MIR type in this file!
> MIR.h should not be included by Recover.h for compilation performance
> reasons. (*)
> 
> Use a uint8_t as storage, and coerce it back in the recover method
> implemented in Recover.cpp, which can include MIR.h.

Enums can be forward-declared now, so you could also do something like

  enum class Foo : uint8_t;
Comment on attachment 8870480 [details] [diff] [review]
bug1367088-part2-inline-newstringiterator.patch

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

::: js/src/jit-test/tests/ion/recover-newstringiterator.js
@@ +7,5 @@
> +
> +    var NewStringIterator = getSelfHostedValue("NewStringIterator");
> +    var iter = NewStringIterator();
> +    bailout();
> +    // assertRecoveredOnBailout(iter, true);

Why is this commented?  Can you add a similar test case in dce-with-rinstruction.js ?
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> ::: js/src/jit-test/tests/ion/recover-newstringiterator.js
> @@ +7,5 @@
> > +
> > +    var NewStringIterator = getSelfHostedValue("NewStringIterator");
> > +    var iter = NewStringIterator();
> > +    bailout();
> > +    // assertRecoveredOnBailout(iter, true);
> 
> Why is this commented?  Can you add a similar test case in
> dce-with-rinstruction.js ?

The test was copied from the existing recover-newarrayiterator.js test case, which also has this line commented out. Tom or Jan should know why we can't call assertRecoveredOnBailout here (maybe bug 1353875?).
Yes exactly, assertRecoveredOnBailout just doesn't work in all jit modes.
(In reply to Tom Schuster [:evilpie] from comment #11)
> Yes exactly, assertRecoveredOnBailout just doesn't work in all jit modes.

Thanks!
Updated to not use MIR types in Recover.h per comment #7. I can't use forward declared types, because C++ doesn't support forward declarations for types in declared in classes. Carrying r+ from evilpie.
Attachment #8870476 - Attachment is obsolete: true
Attachment #8871639 - Flags: review+
Update part 2 to apply cleanly on changed part 1. Carrying r+.
Attachment #8870480 - Attachment is obsolete: true
Attachment #8871640 - Flags: review+
Update part 3 per review comments, carrying r+.
Attachment #8870483 - Attachment is obsolete: true
Attachment #8871641 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d43cb547757f
Part 1: Generalize NewArrayIterator so it can be reused for other iterator types. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/0414ffab16f8
Part 2: Inline NewStringIterator in Ion. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/4769df59b9c2
Part 3: Add inline paths when calling String.fromCodePoint with non-Latin1 code points. r=evilpie
Keywords: checkin-needed
Depends on: 1368344
You need to log in before you can comment on or make changes to this bug.