Closed Bug 1000942 Opened 6 years ago Closed 6 years ago

Eliminate some unnecessary object type barriers

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file)

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();
Attached patch PatchSplinter Review
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)
> 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...
Attachment #8412494 - Flags: review?(bhackett1024) → review+
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/2048240a81d2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1086842
You need to log in before you can comment on or make changes to this bug.