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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

({verified1.9.1})

Other Branch
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1.1 -
in-testsuite +

Firefox Tracking Flags

(blocking1.9.1 .2+, status1.9.1 .2-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

v1
2.45 KB, patch
gal
: review+
brendan
: review+
Samuel Sidler (old account; do not CC)
: approval1.9.1.2+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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"
Flags: blocking1.9.2?
Flags: blocking1.9.1.1?
(Assignee)

Comment 1

9 years ago
Created attachment 387262 [details] [diff] [review]
v1
Attachment #387262 - Flags: review?(gal)
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
Attachment #387262 - Flags: review+

Comment 3

9 years ago
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.
Attachment #387262 - Flags: review?(gal) → review+
(Assignee)

Comment 4

9 years ago
(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...
(Assignee)

Comment 5

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

Apparently this kills us on Fannkuch in the browser -- under Dromaeo, anyway. Investigating.
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?
(Assignee)

Comment 7

9 years ago
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.
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
(Assignee)

Comment 10

9 years ago
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.
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 11

9 years ago
I neglected to run trace-test.js in a debug build.  Stats have changed for the worse in a few tests.  Filed bug.
(Assignee)

Comment 12

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

Er, more specifically, bug 503191.
Depends on: 503191
Someone back me up or fix if I've added one two many deps.

/be
Depends on: 503198
(Assignee)

Updated

9 years ago
No longer depends on: 503198
(Assignee)

Updated

9 years ago
Depends on: 503198

Comment 14

9 years ago
http://hg.mozilla.org/mozilla-central/rev/a08e3540da32
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
blocking1.9.1: --- → .2+
Flags: blocking1.9.1.1? → blocking1.9.1.1-
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?

Updated

9 years ago
Attachment #387262 - Flags: approval1.9.1.2?
Attachment #387262 - Flags: approval1.9.1.2? → approval1.9.1.2+
Comment on attachment 387262 [details] [diff] [review]
v1

Approved for 1.9.1.2. a=ss for release-drivers
(Assignee)

Updated

9 years ago
status1.9.1: --- → .2-fixed
v 1.9.1.2 mac/win shell.
Keywords: verified1.9.1
(Assignee)

Updated

9 years ago
Flags: blocking1.9.2?
trace-test testRebranding2
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.