Inline Object.prototype.toString in Ion

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Frameworks like Angular often do things like:

  var toString = Object.prototype.toString;

  function isDate(obj) {
      return toString.call(obj) === '[object Date]';
  }

There are a lot of variations on this.

The cool thing is that we can use TI in IonBuilder to guard the object (or its proto chain) does not have a Symbol.tostringTag property. Then the toString result only depends on the clasp, so we can either constant fold it or use a MIR instruction that can be folded/loop-hoisted to determine it.

I have patches for this that let us optimize a number of these toString calls on Speedometer. We need better inlining and type information to optimize more of these calls, but I think it's a good start.
(Assignee)

Comment 1

a year ago
Created attachment 8891268 [details] [diff] [review]
Part 1 - Optimize native calls when inlining funcall/funapply

In IonBuilder::jsop_funcall and IonBuilder::jsop_funapplyarguments we currently support only inlining of scripted calls. I don't think there's a good reason why we can't optimize native calls there as well, by calling inlineSingleCall instead of inlineScriptedCall.

This is necessary for the next patch, but it also helps other cases, for instance the micro-benchmark below improves from 54 ms to 11 ms.

function f() {
    var t = new Date;
    var arr = [];
    for (var i = 0; i < 1000000; i++) {
        Array.prototype.push.call(arr, i);
    }
    print(new Date - t);
}
f();
Attachment #8891268 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 2

a year ago
Created attachment 8891293 [details] [diff] [review]
Part 2 - Inline Object.prototype.toString

This inlines Object.prototype.toString when the argument is an object that's not a proxy and does not have a Symbol.toStringTag property.

We can constant-fold the Object/Array/Function cases. I added some logging and these are the known-class cases that show up on Speedometer and Gmail (I also saw some DOM classes but these are a bit harder to fold because we have to get or create an atom, and probably not worth it for now).

There's a lot of room for improvement, for instance the MIR instruction could store an array of classes that can show up and then we can just inline (some of) these cases directly in JIT code instead of doing a VM call, but I think this is a reasonable start.

The micro-benchmark below improves from 180 ms to 9 ms because we can fold the result to "[object Object]":

function f() {
    var arr = [{}, {}, {}, {}];
    var res;
    var t = new Date;
    for (var i = 0; i < 10000000; i++) {
        var o = arr[i % 4];
        res = Object.prototype.toString.call(o);        
    }
    print(new Date - t);
    print(res);
}
f();

If I change |arr| to [{}, new Date, /a/, []] we can no longer fold it but we're still much faster: it improves from 260 to 91 ms.

evilpie, the patch is pretty straight-forward and I think you're most familiar with this code. If you don't have time feel free to cancel the review.
Attachment #8891293 - Flags: review?(evilpies)
(Assignee)

Comment 3

a year ago
Comment on attachment 8891293 [details] [diff] [review]
Part 2 - Inline Object.prototype.toString

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

::: js/src/builtin/Object.cpp
@@ +633,5 @@
> +    const char* className = clasp->name;
> +    StringBuffer sb(cx);
> +    if (!sb.append("[object ") ||
> +        !sb.append(className, strlen(className)) ||
> +        !sb.append("]"))

Note to self: use ']' here instead of "]", similar to what anba fixed in bug 1384244.
Comment on attachment 8891268 [details] [diff] [review]
Part 1 - Optimize native calls when inlining funcall/funapply

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

I cannot think of any issue and wonder why we did not had that before.
Attachment #8891268 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8891293 [details] [diff] [review]
Part 2 - Inline Object.prototype.toString

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

Oh nice, actually quite straightforward as well. Maybe we should self-host the @@toStringTag part of toString and call a (inlineable) intrinsic that does the classname lookup.
Attachment #8891293 - Flags: review?(evilpies) → review+

Comment 6

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/433a0c6cef71
part 1 - Ion-inline native calls in jsop_funcall/jsop_funapplyarguments. r=nbp

Comment 7

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa3fa3b4af72
part 2 - Inline Object.prototype.toString in Ion. r=evilpie

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/433a0c6cef71
https://hg.mozilla.org/mozilla-central/rev/aa3fa3b4af72
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.