Closed
Bug 1367088
Opened 8 years ago
Closed 8 years ago
Improve StringIterator performance
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
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)
18.37 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
15.46 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
9.86 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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 7•8 years ago
|
||
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.
Comment 8•8 years ago
|
||
(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 9•8 years ago
|
||
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•8 years 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?).
Comment 11•8 years ago
|
||
Yes exactly, assertRecoveredOnBailout just doesn't work in all jit modes.
Assignee | ||
Comment 12•8 years 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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Update part 3 per review comments, carrying r+.
Attachment #8870483 -
Attachment is obsolete: true
Attachment #8871641 -
Flags: review+
Assignee | ||
Comment 16•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=616accdfbd934621a99cdd2bea108a72db2095ed
Keywords: checkin-needed
Comment 17•8 years 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•8 years 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
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•