avoid creating NumberObject in (.1).toString

RESOLVED FIXED in mozilla25



JavaScript Engine
5 years ago
4 years ago


(Reporter: luke, Assigned: Yaron Tausky)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [good first bug][js:t])


(1 attachment, 2 obsolete attachments)



5 years ago
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; } });
  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?
Whiteboard: [good first bug] → [good first bug][js:t]
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.
I am interested in working on this bug, although I can't see where ValueToObject is called within GetPropertyOperation.
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.  :-)
Could you provide any guidance on how to detect if toString() is being called?

Comment 5

4 years ago
Since there's been little activity on this bug lately, I'd like to work on it.

Comment 6

4 years ago
Created attachment 778037 [details] [diff] [review]

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.
Attachment #778037 - Flags: review?(luke)

Comment 7

4 years ago
Comment on attachment 778037 [details] [diff] [review]

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

4 years ago
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

4 years ago
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

4 years ago
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

4 years ago
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. :)
Attachment #778037 - Attachment is obsolete: true
Attachment #778037 - Flags: review?(luke)
Attachment #778134 - Flags: review?(luke)

Comment 12

4 years ago
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 :)
Attachment #778134 - Flags: review?(luke) → review+
(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

4 years ago
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.
Ah, that makes sense. I was just wondering if there's some subtlety in our rules that I'm missing.

Comment 16

4 years ago
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.


4 years ago
Whiteboard: [good first bug][js:t] → [good first bug][js:t][checkin-needed]


4 years ago
Keywords: checkin-needed
Whiteboard: [good first bug][js:t][checkin-needed] → [good first bug][js:t]


4 years ago
Assignee: general → yaron.tausky
Attachment #778134 - Attachment is obsolete: true
Keywords: checkin-needed
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.