Closed
Bug 579479
Opened 14 years ago
Closed 14 years ago
JM: Make single-char string comparisons fast
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dmandelin, Assigned: bhackett1024)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Make this fast:
function f() {
var s = "a", t = "b";
var t0 = new Date;
for (var i = 0; i < 233457; ++i) {
s == t;
}
print(new Date - t0);
}
f();
The optimization should work regardless of whether the operands expressions are variables or literals. We do 233457 single-character string compares in SS.
JM wanted SS win: 3 ms
Comment 1•14 years ago
|
||
Taking this one and merging it with the string == and === work I was planning on doing.
Assignee: general → cdleary
Status: NEW → ASSIGNED
Comment 2•14 years ago
|
||
Talked to bhackett about this, handing it over to go work on Yarr stuff.
Assignee: cdleary → bhackett1024
Assignee | ||
Comment 3•14 years ago
|
||
This patch adds an OOL fast path for equality/disequality on strings. Only atomized strings are compared; NewDependentString is modified to return atomized unit strings where possible, saving 11k allocations on date-format-tofte. Saves 5.6ms for me on tofte, still failing a few regressions. For this microbenchmark:
function foo() {
var a = "a";
var b = "b";
var j = 0;
for (var i = 0; i < 2000000; i++) {
if (a == b)
j++;
}
}
foo();
I get these times:
JSC: 35ms
V8: 14ms
JM (old): 48ms
JM (new): 10ms
Assignee | ||
Comment 4•14 years ago
|
||
jstests and trace-tests pass with this patch.
Attachment #465988 -
Attachment is obsolete: true
Attachment #465998 -
Flags: review?(dvander)
Comment on attachment 465998 [details] [diff] [review]
patch fixing regressions
Sorry for the delay here.
>+ RegisterID lr = frame.ownRegForData(lhs);
>+ RegisterID rr = Registers::ReturnReg;
>+ if (rhs->isConstant()) {
>+ rhsConst = true;
>+ rval = rhs->getValue();
>+ } else {
>+ rr = frame.ownRegForData(rhs);
>+ }
We can use MaybeRegsterID here instead of the bogus initializations (the old code was written before that existed).
>+ RegisterID lt = Registers::ReturnReg;
Ditto.
>+ /* Write values explicitly for the two stack slots. */
We should avoid using addressOf() outside of FrameState. Sometimes it's a necessary evil but here there are two red flags: it's a lot of code (18 lines), and it could lead to duplicate syncs. However, I think I see what you're working around: if we're smarter about syncing the top two entries, we can avoid emitting the whole frame sync code in two different places. A few solutions in no particular order (though I like 2 best):
1) Don't worry about it and just sync the frame in two places - what we were doing originally.
2) Introduce something like FrameState::popAndUse() which grabs own registers, fills an outparam struct, then pops the value. Then introduce another function, like FrameState::syncFromInfo(). The struct would look something like ValueRemat in RematInfo.h, though you'd want bits describing the sync states and the Address.
3) Use FrameState::allocForBinary with some tweaking - you don't want a result reg. If you pin its output registers then you can safely call forgetEverything() and use them after. Downside is that you might get some stores on the fast-path.
4) Introduce a new function, FrameState::syncBelow(Uses uses), which does not sync or evict N top entries. This could be implemented by pinning the registers for the FEs and then calling sync().
The rest of the patch looks okay, but I'd prefer we improve FrameState rather than hack around it. There are most likely better solutions still so please let me know if you have other ideas or if I've completely misread your use case.
Attachment #465998 -
Flags: review?(dvander)
Assignee | ||
Comment 6•14 years ago
|
||
I think this is closest to solution 2, borrowing bits from solution 4.
Attachment #465998 -
Attachment is obsolete: true
Attachment #470073 -
Flags: review?(dvander)
Comment on attachment 470073 [details] [diff] [review]
patch fixing comments
Thanks for enhancing FrameState. One question:
>+ stubcc.jumpInScript(matched, target);
>+ stringFallthrough = stubcc.masm.jump();
Does this bypass the tracing hook? It looks like it. You may want to forward all positive stub branches to one label, then use a jump there for jumpAndTrace().
r=me w/ that, and sorry for late review (again).
Attachment #470073 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 8•14 years ago
|
||
This adds a second optional trace hint for slow path backwards jumps, which should be faster than doing a second jump.
Attachment #470073 -
Attachment is obsolete: true
Attachment #475151 -
Flags: review?(dvander)
Updated•14 years ago
|
Attachment #475151 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 9•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/10d8a3d57004
Looks like 7.5ms on SS on AWFY. (will watch subsequent checkins, I get a bigger improvement testing locally).
Comment 10•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Fix for --disable-tracejit --enable-methodjist bustage:
http://hg.mozilla.org/tracemonkey/rev/8b875d1bc70d
You need to log in
before you can comment on or make changes to this bug.
Description
•