575766 - JM: Speed up JSOP_AND, JSOP_OR.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Shark data for SunSpider revealed that 1.7% of execution time under JaegerMonkey was spent within js_ValueToBoolean(). I therefore counted the # of occurrences of types passed into stubs::ValueToBoolean(), with the following results:

 int32      936 493
 boolean    741 477
 string      23 323
 object       4 312
 null/undef   3 541
 double           1

Similarly, I also counted the same data for js_ValueToBoolean() when using only the interpreter:

 int32      936 493
 string      23 331
 object       5 205
 null/undef   3 099
 double           1
 boolean          0

Based on the above frequencies, I reordered the type checks within js_ValueToBoolean() for a cheap win (~3ms locally on SunSpider; no test slowed), and inserted a fast-path for boolean for JSOP_ADD and JSOP_OR, which were responsible for the majority of calls to ValueToBoolean().

For the v8 benchmark, stubs::ValueToBoolean() is called with the following type frequencies in the context of JSOP_ADD and JSOP_OR:

 boolean    3 197 366
 int32         16 766
 null/undef     8 385
 object             8
 string             0
 double             0

The fast-path for boolean eliminates all 3.2 million calls and much of the syncing work, for the cost of two branches in the fast path and three in the ool path. In most tests, this gets faster; in raytracer, the OOL path for conversion is more frequently taken, resulting in ~30ms slowdown locally. In total with the patch, v8 becomes ~100ms faster: about 2%.

Comment 1

[Sean Stangl [:sstangl]]

Attachment: Reorder js_ValueToBoolean() checking order for a small win.

Comment 2

[Sean Stangl [:sstangl]]

Attachment: Fast-path boolean for JSOP_AND, JSOP_OR.

Comment 3

[Sean Stangl [:sstangl]]

Depending on FrameState changes to eliminate frame.forgetEverything() call, which should give a much nicer perf score.

Comment 4

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/523ad9763908

Comment 5

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/568850e1f3a1

Pushed the patch that was actually r+'d. That explains the surprise when I pushed the other patch!

This bug is left open since everything can be dramatically improved if forgetEverything() goes away.

Comment 6

[David Mandelin [:dmandelin]]

> This bug is left open since everything can be dramatically improved if
> forgetEverything() goes away.

I'm gonna close this out to keep things simple. Feel free to file a followup bug on forgetEverything if it's separate from planned register allocation work.

Comment 7

[Paul Wright]

Just a ping... did forgetEverything() go away?

Comment 8

[Sean Stangl [:sstangl]]

It did not -- see http://hg.mozilla.org/users/danderson_mozilla.com/moo/file/e0988eae6c08/js/src/methodjit/FastOps.cpp#l749 .

frame.forgetEverything() must currently be called on both sides of a JS jump, since the FrameState (tracking our stack and registers) in both locations must be equivalent. The easiest way to make them equivalent is to remove all known data and reload from memory.

FrameState changes are tracked in bug 574930.

Chris Leary and I did a bit of work on a possible solution to FrameState branching/merging, but it's heavily incomplete: https://hg.mozilla.org/users/sean.stangl_gmail.com/jitpaths/ .