Closed
Bug 1385215
Opened 7 years ago
Closed 7 years ago
Inline Object.prototype.toString in Ion
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files)
2.27 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
22.55 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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 4•7 years ago
|
||
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+
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
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/433a0c6cef71
https://hg.mozilla.org/mozilla-central/rev/aa3fa3b4af72
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•