Improve StringIterator performance

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year 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)

(Assignee)

Description

a year ago
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: 1307395
(Assignee)

Comment 2

a year ago
Created attachment 8870476 [details] [diff] [review]
bug1367088-part1-generalize-newarrayiterator-codegen.patch

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

Comment 3

a year ago
Created attachment 8870480 [details] [diff] [review]
bug1367088-part2-inline-newstringiterator.patch

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

Comment 4

a year ago
Created attachment 8870483 [details] [diff] [review]
bug1367088-part3-inline-fromcodepoint-non-latin1.patch

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

Comment 10

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

Comment 12

a year ago
(In reply to Tom Schuster [:evilpie] from comment #11)
> Yes exactly, assertRecoveredOnBailout just doesn't work in all jit modes.

Thanks!
(Assignee)

Comment 13

a year ago
Created attachment 8871639 [details] [diff] [review]
bug1367088-part1-generalize-newarrayiterator-codegen.patch

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

Comment 14

a year ago
Created attachment 8871640 [details] [diff] [review]
bug1367088-part2-inline-newstringiterator.patch

Update part 2 to apply cleanly on changed part 1. Carrying r+.
Attachment #8870480 - Attachment is obsolete: true
Attachment #8871640 - Flags: review+
(Assignee)

Comment 15

a year ago
Created attachment 8871641 [details] [diff] [review]
bug1367088-part3-inline-fromcodepoint-non-latin1.patch

Update part 3 per review comments, carrying r+.
Attachment #8870483 - Attachment is obsolete: true
Attachment #8871641 - Flags: review+

Comment 17

a year ago
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

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d43cb547757f
https://hg.mozilla.org/mozilla-central/rev/0414ffab16f8
https://hg.mozilla.org/mozilla-central/rev/4769df59b9c2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1368344
You need to log in before you can comment on or make changes to this bug.