We seem to be hitting a stubs::Mod slow path on this testcase

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: bz, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(1 attachment)

Takes up about 11% of the time.  More than half of that is js::ValueToNumberSlow (good name!).

The testcase is also attached in bug 594230 if it ever goes away.
This testcase has this code:
Number.prototype.m = function (a) {
    return ((this % a) + a) % a
};
Which makes everything really slow, because of the the number object accessed via 'this'.
I implemented a fast path for number objects in DefaultValue exactly like these for string objects.

for (i = 0; i <= 1000000; i++) {
    i.m(a);
}

This micro benchmark improved from 486 to 343 with js -m.
Created attachment 483931 [details] [diff] [review]
fast path for numbers object in DefaultValue
Attachment #483931 - Flags: review?(lw)
Comment on attachment 483931 [details] [diff] [review]
fast path for numbers object in DefaultValue

Great job! r+ with nits:

>+            ClassMethodIsNative(cx, obj,
>+                                 &js_StringClass,
>                                  ATOM_TO_JSID(cx->runtime->atomState.toStringAtom),
>                                  js_str_toString)) {

Nit: need to fixup the indentation here and below.

>+        /* Optimize (new Number(...)).valueOf(). */
>+        if (obj->getClass() == &js_NumberClass &&
>+            ClassMethodIsNative(cx, obj,
>+                                 &js_NumberClass,
>+                                 ATOM_TO_JSID(cx->runtime->atomState.valueOfAtom),
>+                                 js_num_valueOf)) {

Nit: could you factor out the obj->getClass() between the js_StringClass/js_NumberClass cases into a local var and put both the string/number cases in a single compound if?
Attachment #483931 - Flags: review?(lw) → review+
bz, does this patch take us to where we want to be?  To go further we could do something with PICs so that we never leave jit code.
Well, I dunno where we want to be.  ;)

Without this patch, time under stubs::Mod is 14% of total on m-c tip.  With this patch, it's 7% of total.  So better, for sure.  The fmod time is the same for both (2.4%), the self time is the same (1.2%), js_ValueToNumberSlow went from 10.5% to 2.9% (mostly searching property tables and self time in DefaultValue).

I suspect short of not having a stub call here at all we're pretty good...  I don't know whether avoiding the stub call in this situation is reasonable or desirable.
http://hg.mozilla.org/tracemonkey/rev/759533cf3c38

To wit, I get a 3x speedup (-m) with this patch on

  var n = new Number(3.4);
  for (var i = 0; i < 1000000; ++i) {
      n + n;
  }
Whiteboard: fixed-in-tracemonkey

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/759533cf3c38
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.