Closed Bug 579479 Opened 14 years ago Closed 14 years ago

JM: Make single-char string comparisons fast

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dmandelin, Assigned: bhackett1024)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Taking this one and merging it with the string == and === work I was planning on doing.
Assignee: general → cdleary
Status: NEW → ASSIGNED
No longer depends on: 460822
Talked to bhackett about this, handing it over to go work on Yarr stuff.
Assignee: cdleary → bhackett1024
Attached patch WIP strcmp patch (obsolete) — Splinter Review
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
Attached patch patch fixing regressions (obsolete) — Splinter Review
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)
Attached patch patch fixing comments (obsolete) — Splinter Review
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+
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)
Attachment #475151 - Flags: review?(dvander) → review+
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).
http://hg.mozilla.org/mozilla-central/rev/10d8a3d57004
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.