Closed
Bug 1000942
Opened 10 years ago
Closed 10 years ago
Eliminate some unnecessary object type barriers
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file)
63.30 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
See the micro-benchmark below. Obj.x has TypeSet: {null, 4 TypeObjects} The read has a type barrier that guards it's an object (this is good), but it also guards the object type is one of the 4 TypeObjects. The latter is unnecessary; if it's an object it's guaranteed to have one of these TypeObjects. function Obj() { this.x = null; } function f() { var arr = [new Obj, new Obj, new Obj, new Obj]; arr[0].x = {}; arr[1].x = {}; arr[2].x = {}; arr[3].x = {}; for (var i=0; i<10000; i++) { var res = arr[i % 4].x; } return res; } f();
Assignee | ||
Comment 1•10 years ago
|
||
This patch turns the "bool barrier" we have in many places into an enum, BarrierKind (strongly-typed enum so the conversion was pretty straight-forward). The enum has 3 possible values: - NoBarrier (the old "barrier = false") - TypeSet (the old "barrier = true") - TypeTagOnly, which means the barrier only has to check the tag and can ignore specific object types. I added some logging and this avoids object guards on pretty much every Octane benchmark. On Octane-richards, it wins at least 1000 points. Also, the object-or-null pattern is pretty common in JS code, so I think avoiding object type checks there is nice for real-world code too. Furthermore, this enum should make it easier to add other barrier types in the future (we could guard on the object Class etc).
Attachment #8412494 -
Flags: review?(bhackett1024)
Comment 2•10 years ago
|
||
> we could guard on the object Class etc
This sounds like it could be used for our Ion bits where we really want to guard on "is a Node", not "is in this list of Node types", too. I thought we had a bug on that, but I can't find it...
Updated•10 years ago
|
Attachment #8412494 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 3•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7e09bbee26 Linux x64 Try: https://tbpl.mozilla.org/?tree=Try&rev=a61c26fda154
Comment 4•10 years ago
|
||
backed this out for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=38689784&tree=Mozilla-Inbound
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #4) > backed this out for test failures like > https://tbpl.mozilla.org/php/getParsedLog.php?id=38689784&tree=Mozilla- > Inbound Investigating now. Looks like it's a pre-existing issue that's uncovered by this patch.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5) > Investigating now. Looks like it's a pre-existing issue that's uncovered by > this patch. Nope, I was missing a freeze constraint. This optimization is only valid if we unsure no new object types are added for the property. https://tbpl.mozilla.org/?tree=Try&rev=087d1bd7501b
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2048240a81d2
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2048240a81d2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•