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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Till Schneidereit [:till]]

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.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Add an assert, fix a bug

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();

Comment 3

[Jan de Mooij [:jandem]]

(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.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Don't reshape proto chain if object is not a proto

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Fix JSObject::splicePrototype

Browser calls JS_SplicePrototype -> JSObject::splicePrototype, which didn't set the delegate flag.

Comment 6

[Jan de Mooij [:jandem]]

Folded the patches because they are so small:

https://hg.mozilla.org/integration/mozilla-inbound/rev/130592a09462

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/130592a09462

Comment 8

[Hannes Verschore [:h4writer]]

AWFY agrees: 541 to 1789 on raytrace.swf. Nice!