Closed Bug 1367088 Opened 8 years ago Closed 8 years ago

Improve StringIterator performance

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 3 obsolete files)

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.

Attachment

General

Created:
Updated:
Size: