502890 - Assigning to a called method on trace does not change the object's shape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

var x = "FAIL";
function f1(){}
function f2(){ x = "PASS"; }

obj = {m: f1};

var arr = [f1, f1, f1, f2];
for (var i = 0; i < 4; i++) {
    obj.m = arr[i];  // obj.m=f2 should change obj's shape but doesn't
    obj.m();
}
assertEq(x, "PASS");

Without -j this passes. With -j,
crasher.js:15: TypeError: Assertion failed: got "FAIL", expected "PASS"

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 387262 [details] [diff] [review]
v1

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -9011,32 +9011,32 @@ TraceRecorder::record_SetPropHit(JSPropC
>     JSScope* scope = OBJ_SCOPE(obj);
> 
>     JS_ASSERT(scope->object == obj);
>     JS_ASSERT(SCOPE_HAS_PROPERTY(scope, sprop));
> 
>     if (!isValidSlot(scope, sprop))
>         return JSRS_STOP;
> 
>+    /*
>+     * Setting a function-valued property might need to rebrand the object; we

..12345678901234567890123456789012345678901234567890123456789012345678901234567890
>+     * don't trace that case. There's no need to guard on that, though, because

Lose the French spacing and because fits on the second real line of the comment ;-).

>+     * because separating functions into the trace-time type TT_FUNCTION will
>+     * save the day!
>+     */
>+    if (VALUE_IS_FUNCTION(cx, r))
>+        ABORT_TRACE("can't trace function-valued property set");

Any effects on v8, ss, or dromaeo?

Drive-by r+ but let's hear about abort count deltas for these "benchmarks" first.

/be

Comment 3

[Andreas Gal :gal]

Comment on attachment 387262 [details] [diff] [review]
v1

I wonder whether this is going to bite us somewhere and we fall off trace in a perf critical case. Probably not on SS and V8 bench, but Dromeao loves to do weird things to closures and objects.

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> Any effects on v8, ss, or dromaeo?

No effect on v8 or ss. Shutting down now for a couple Dromaeo runs...

Comment 5

[Jason Orendorff [:jorendorff]]

http://dromaeo.com/?id=70152,70154

Apparently this kills us on Fannkuch in the browser -- under Dromaeo, anyway. Investigating.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Jason, can you add a test that's one shorter than your testRebranding2 and doesn't have the 'h' in the array and that asserts via jitstats that it traces (that is, that the array length is > RUNLOOP, effectively).

Or just test as part of this test that the array length > RUNLOOP?

Comment 7

[Jason Orendorff [:jorendorff]]

Hmm. Fannkuch runs just fine in the browser under the SunSpider harness at
  http://www2.webkit.org/perf/sunspider-0.9/sunspider.html
Same speed with or without the patch.  Trying Dromaeo again.

Comment 8

[Jason Orendorff [:jorendorff]]

Yes, it was just a fluke.
  http://dromaeo.com/?id=70158,70160

Pushed.
  http://hg.mozilla.org/tracemonkey/rev/a08e3540da32

And oops, here's the change requested in comment 6:
  http://hg.mozilla.org/tracemonkey/rev/ea09ea5a5eeb

Comment 9

[Brendan Eich [:brendan]]

Does Dromaeo have too many flukes (liver flukes :-P)? It seems (correct me if I'm being unfair) that we have found cases where it tests closure optimizations, or setting and deleting global props, or other things than what the particular SS test it hacks up to rate-measure purports to test.

/be

Comment 10

[Jason Orendorff [:jorendorff]]

In this particular case I don't see how Dromaeo can be blamed. Probably some random time-wasting event on my computer interfered with the benchmark.

Comment 11

[Jason Orendorff [:jorendorff]]

I neglected to run trace-test.js in a debug build.  Stats have changed for the worse in a few tests.  Filed bug.

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to comment #11)
> Filed bug.

Er, more specifically, bug 503191.

Comment 13

[Brendan Eich [:brendan]]

Someone back me up or fix if I've added one two many deps.

/be

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a08e3540da32

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob: is this ready to go on mozilla-1.9.1 for Firefox 3.5.2? If so, please mark the patch with approval1.9.1.2?

Comment 16

[Samuel Sidler (old account; do not CC)]

Comment on attachment 387262 [details] [diff] [review]
v1

Approved for 1.9.1.2. a=ss for release-drivers

Comment 17

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/881bd722cdfc

Comment 18

[Bob Clary [:bc] (inactive)]

v 1.9.1.2 mac/win shell.

Comment 19

[Bob Clary [:bc] (inactive)]

trace-test testRebranding2