Closed Bug 1125505 Opened 5 years ago Closed 5 years ago

SetClassAndProto should only reshape the proto chain if the object is a proto

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(3 files)

SetClassAndProto reshapes all objects on the proto chain. This is necessary for the teleporting optimization (which we can probably remove, but that's for another bug). For instance:

  var A = {x: 3};
  var B = Object.create(A);
  var C = Object.create(B);
  var D = Object.create(C);
  assertEq(D.x, 3);
  C.__proto__ = {x: 10};
  assertEq(D.x, 10);       // Relies on reshaping of A.

It seems to me we only need to reshape the proto chain if the object is on the proto chain of another object (obj->isDelegate()). If the object is *not* a proto, reshaping is not necessary because caches will always guard on the object's shape (which will still change).

This is good enough for the mutable __proto__ case I saw in Shumway (see bug 1108444) and raytrace.swf becomes 3x faster.

CC'ing some people, let me know if I'm missing something.
Summary: SetClassAndProto should only reshape the proto chain if the object is not a proto → SetClassAndProto should only reshape the proto chain if the object is a proto
Blocks: 1103441
Excellent!

While I'm finally working on getting rid of all those ungainly __proto__ mutations in Shumway, I'm pretty sure that there's quite a bit of code out there that will be sped up by this. It might well help with a lot of Traceur-compiled code, for example: see bug 1049041.
This patch asserts proto->isDelegate() in getProto(). This uncovered an old bug: SetClassAndProto didn't call setDelegate on the new proto. This breaks teleporting, see the testcase below.

var A = {x: 1};
var B = Object.create(A);
var C = {};
C.__proto__ = B;
function f() {
    for (var i=0; i<25; i++) {
        assertEq(C.x, (i <= 20) ? 1 : 3);
        if (i === 20) {
            B.x = 3;
        }
    }
}
f();
Attachment #8554159 - Flags: review?(bhackett1024)
(In reply to Jan de Mooij [:jandem] from comment #2)
> This uncovered an old
> bug: SetClassAndProto didn't call setDelegate on the new proto. This breaks
> teleporting, see the testcase below.

I can repro this on ESR24 (it's probably much older than that even), so not a recent regression.
Depends on: CVE-2015-0820
Browser calls JS_SplicePrototype -> JSObject::splicePrototype, which didn't set the delegate flag.
Attachment #8554183 - Flags: review?(bhackett1024)
Attachment #8554159 - Flags: review?(bhackett1024) → review+
Attachment #8554164 - Flags: review?(bhackett1024) → review+
Attachment #8554183 - Flags: review?(bhackett1024) → review+
Folded the patches because they are so small:

https://hg.mozilla.org/integration/mozilla-inbound/rev/130592a09462
https://hg.mozilla.org/mozilla-central/rev/130592a09462
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
AWFY agrees: 541 to 1789 on raytrace.swf. Nice!
You need to log in before you can comment on or make changes to this bug.