JM: Speed up JSOP_AND, JSOP_OR.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sstangl, Assigned: sstangl)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
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%.
(Assignee)

Comment 1

8 years ago
Created attachment 454943 [details] [diff] [review]
Reorder js_ValueToBoolean() checking order for a small win.
Attachment #454943 - Flags: review?(dvander)
(Assignee)

Comment 2

8 years ago
Created attachment 454945 [details] [diff] [review]
Fast-path boolean for JSOP_AND, JSOP_OR.
Attachment #454945 - Flags: review?(dvander)
(Assignee)

Comment 3

8 years ago
Depending on FrameState changes to eliminate frame.forgetEverything() call, which should give a much nicer perf score.
Depends on: 574930
Attachment #454943 - Flags: review?(dvander) → review+
(Assignee)

Comment 5

8 years ago
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.
Attachment #454945 - Flags: review?(dvander) → review+
> 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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 7

8 years ago
Just a ping... did forgetEverything() go away?
(Assignee)

Comment 8

8 years ago
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/ .
You need to log in before you can comment on or make changes to this bug.