Last Comment Bug 771865 - avoid creating NumberObject in (.1).toString
: avoid creating NumberObject in (.1).toString
Status: RESOLVED FIXED
[good first bug][js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla25
Assigned To: Yaron Tausky
:
Mentors:
Depends on:
Blocks: 771737
  Show dependency treegraph
 
Reported: 2012-07-07 20:40 PDT by Luke Wagner [:luke]
Modified: 2013-07-23 17:49 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug771865.patch (1.72 KB, patch)
2013-07-18 13:39 PDT, Yaron Tausky
no flags Details | Diff | Review
bug771865.patch (v2) (2.29 KB, patch)
2013-07-18 15:54 PDT, Yaron Tausky
luke: review+
Details | Diff | Review
bug771865.patch (v3) (4.07 KB, patch)
2013-07-19 07:27 PDT, Yaron Tausky
no flags Details | Diff | Review

Description Luke Wagner [:luke] 2012-07-07 20:40:55 PDT
Calling toString on a primitive number value (i.e., where v.isNumber()) involves allocating a NumberObject GC thing which we only use for property lookup and then throw away.  We shouldn't have to do this in the common case where we are just calling builtin methods of Number.prototype (like toString).  (This came up in the context of bug 771737.)

Specifically: in GetPropertyOperation (jsinterpinlines.h), we call ValueToObject before doing the property lookup.  I think the fix is to do something like js::DefaultValue does using ClassMethodIsNative(..., num_toString).  In particular, we need to watch out that we don't break:

  var getterThis;
  Object.defineProperty(Number.prototype, 'g', { get:function() { getterThis = this; } });
  (.1).g;
  assertEq(getterThis instanceof Number, true);
  assertEq(getterThis === Number.prototype, false);

Fortunately, iiuc, we don't need to worry about content mutating Number.prototype:

  Number.prototype = 42;
  assertEq((.1).toString(), "0.1");

Any other hazards Waldo?
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-09 17:39:01 PDT
Really we should just be able to guard on the identity of Number.prototype.toString not changing, then we can perform a function call exactly as we usually would, passing along the unadulterated |this| value specified for the call.  JITs could then see that a particular native was being called, and they could generate code customized to the known type of the provided |this|.  There's no reason to box anything at all -- functions that want a boxed |this| should be doing it themselves.  (Well, we have to box in the [hate] propertyop case, but we could probably defer boxing until the very last moment to avoid penalizing everything else.)

This stuff seems not quite like a simple jsinterpinlines.h fix to me, but maybe I'm missing something.
Comment 2 James Kitchener (:jkitch) 2012-09-21 20:51:38 PDT
I am interested in working on this bug, although I can't see where ValueToObject is called within GetPropertyOperation.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-09-22 01:01:26 PDT
It looks like the new relevant method that converts the primitive to an object is ToObjectFromStack.

I'm still kind of thinking the last thing i said in comment 1 is correct, but I'm more than happy to offer what advice I can and eventually be proven wrong.  :-)
Comment 4 James Kitchener (:jkitch) 2012-09-23 05:45:28 PDT
Could you provide any guidance on how to detect if toString() is being called?
Comment 5 Yaron Tausky 2013-07-18 08:06:36 PDT
Since there's been little activity on this bug lately, I'd like to work on it.
Comment 6 Yaron Tausky 2013-07-18 13:39:30 PDT
Created attachment 778037 [details] [diff] [review]
bug771865.patch

I made a preliminary patch -- I think it solves the problem at hand, but I don't know about its quality.
Caveat: I got 5 failures in one of the test suites, but I also get them without the patch applied, so I assume it's due to something else.
Comment 7 Luke Wagner [:luke] 2013-07-18 14:07:51 PDT
Comment on attachment 778037 [details] [diff] [review]
bug771865.patch

Review of attachment 778037 [details] [diff] [review]:
-----------------------------------------------------------------

Great patch, I think you've found the right cut point!  Just a few changes requested below:

Do you suppose you could put in a printf() (temporarily), build a browser and check whether the optimized path you added is hitting for the testcase in bug 771737 comment 0?  If it is hitting, it'd be fun to (take out the printf()) and see if you can see any perf improvement.  Note: we are now much faster than v8 b/c of other optimizations made by Hannes, but I'd be excited to see if this makes us still faster :)

Of course, to go even faster, you could consider optimizing this in Ion so that we could call NumberToString directly and skip all the get-prop business :)

::: js/src/vm/Interpreter.cpp
@@ +264,4 @@
>              return true;
>      }
>  
> +    JSObject *obj = NULL;

In general, we try to minimize our use of raw JSObject* (since these can be invalidated by GC and it requires careful auditing to check that there are no GC hazards).  Instead, could you make 'obj' be a RootedObject (RootedObject obj(cx) constructs a null pointer) and just remove 'nobj' below and replace uses of 'nobj' with 'obj'?  (Really, this code should have been written this way initially.)  If you are unsure of the change, feel free to rerequest review.

@@ +269,5 @@
> +    RootedId id(cx, NameToId(script->getName(pc)));
> +
> +    /* Optimize (.1).toString(). */
> +    if (lval.isNumber() && id == NameToId(cx->names().toString)) {
> +        JSObject *proto = fp->global().getOrCreateNumberPrototype(cx);

The "get" in the name and the "cx" parameter indicate that this operation can fail (in which case it returns NULL).  Can you check for NULL here?

@@ +271,5 @@
> +    /* Optimize (.1).toString(). */
> +    if (lval.isNumber() && id == NameToId(cx->names().toString)) {
> +        JSObject *proto = fp->global().getOrCreateNumberPrototype(cx);
> +        if (ClassMethodIsNative(cx, proto, &NumberObject::class_, id, num_toString)) {
> +            obj = proto;

SM style (https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style) requires you not to put { } around a single-line then-block.
Comment 8 Yaron Tausky 2013-07-18 14:45:51 PDT
Sure, I'll try it in the browser and look into doing it in Ion too.
Should |proto| also be wrapped with a RootedObject?
Comment 9 Luke Wagner [:luke] 2013-07-18 15:03:24 PDT
Great question!  You could, however since ClassMethodIsNative indicates that it doesn't GC by taking a JSObject* (instead of a HandleObject, which is what GC'able methods usually take), and since proto is really narrowly-scoped, it would also be fine to leave it out (Rooted's incur a small amount of runtime overhead).
Comment 10 Luke Wagner [:luke] 2013-07-18 15:14:01 PDT
Also, just to clarify in case I gave the wrong impression: Ion optimizations could definitely be done separately in a followup bug, this bug we could keep scoped to just the patch you've posted.
Comment 11 Yaron Tausky 2013-07-18 15:54:06 PDT
Created attachment 778134 [details] [diff] [review]
bug771865.patch (v2)

Okay, I think it would be more convenient to split the Ion optimisations to another bug. I changed the patch according to your comments, I hope it addresses everything. By the way, according to the coding standard, since I made num_toString external, it should be prefixed with js_. Would you like me to do so?
I'll figure out how to build Firefox tomorrow and report my findings, it's getting late over here. :)
Comment 12 Luke Wagner [:luke] 2013-07-18 16:05:59 PDT
Comment on attachment 778134 [details] [diff] [review]
bug771865.patch (v2)

Yes, js_num_toString would be great.  Again, great first patch; let us know on #jsapi if you want to find a second :)
Comment 13 Till Schneidereit [:till] 2013-07-19 03:09:32 PDT
(In reply to Luke Wagner [:luke] from comment #12)
> Comment on attachment 778134 [details] [diff] [review]
> bug771865.patch (v2)
> 
> Yes, js_num_toString would be great.  Again, great first patch; let us know
> on #jsapi if you want to find a second :)

Mmh, shouldn't that be js::num_toString, nowaways?
Comment 14 Luke Wagner [:luke] 2013-07-19 07:16:01 PDT
It could, but there is already js_num_valueOf right above, so I considered updating these names to use namespaces instead a separate aesthetic change.
Comment 15 Till Schneidereit [:till] 2013-07-19 07:24:06 PDT
Ah, that makes sense. I was just wondering if there's some subtlety in our rules that I'm missing.
Comment 16 Yaron Tausky 2013-07-19 07:27:13 PDT
Created attachment 778465 [details] [diff] [review]
bug771865.patch (v3)

I changed the name to js_num_toString. I also tested it in the browser: the test case Luke mentioned only had 86 hits (and consistently took 6-8ms), so I don't think it has a great effect on it. Ion probably kicks in after that?
As for the formatting, I added a User line and a commit message and modified the diff parameters according to the guide, I hope it's in the okay now.
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-07-23 06:26:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb96c3abe9cd
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-07-23 17:49:53 PDT
https://hg.mozilla.org/mozilla-central/rev/fb96c3abe9cd

Note You need to log in before you can comment on or make changes to this bug.