454039 - TM: don't abort TraceRecorder::record_JSOP_SETPROP on cache miss

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

This would be the first time evolving an object from shape X to X' for the given pc, since the record method runs before the interpreter case. We really want to run after the op, or in the middle. I will try an ad-hoc record_SetProp method, a la EnterFrame and LeaveFrame.

This bites v8/raytrace.js, FWIW.

/be

Comment 1

[Brendan Eich [:brendan]]

Elsewhere we try to funnel through test_property_cache and have it pre-fill the cache to get the right fast-lookup info and prime the cache for the interpreter case that follows the record method. Doing that here is messy cuz of the open-coded specialization in js_Interpret's JSOP_SETPROP.

OTOH we have issues already not doing that for JSOP_SETNAME (see bug 452565). So it might be best to resync interpreter and recorder, which means specializing both record_JSOP_SETPROP and record_JSOP_SETNAME to pre-fill only what their interpreter case counterparts fill.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Leaving JSOP_SETNAME for the other bug to keep patches smaller.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 337399 [details] [diff] [review]
fix

>+++ b/js/src/jsinterp.cpp
>@@ -2570,8 +2570,8 @@
>     /* We had better not be entering the interpreter from JIT-compiled code. */
>     TraceRecorder *tr = NULL;
>     if (JS_TRACE_MONITOR(cx).onTrace) {

Since you're here, want to smack this to be JS_ON_TRACE?

>-        tr = JS_TRACE_MONITOR(cx).recorder;
>-        JS_TRACE_MONITOR(cx).recorder = NULL;
>+        tr = TRACE_RECORDER(cx);
>+        SET_TRACE_RECORDER(cx, NULL);
>     }
> #endif    

Super-nit: remove the trailing whitespace after the endif.

Comment 4

[Brendan Eich [:brendan]]

Fixed on tracemonkey:

http://hg.mozilla.org/tracemonkey/rev/cf7e24068af1

/be

Comment 5

[Brendan Eich [:brendan]]

Backed out. Something broke -- see bug 453918 comment 15 (and 16).

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: fixed fix

Sigh, was in too much of a hurry to land a smaller patch. This one eviscerates record_JSOP_SETNAME, since it is handled by record_SetPropHit too -- and of course the latter must deal with global variables on the native stack, not boxed into property memory.

I hope bugzilla interdiff works. If not, I'll throw up a good interdiff.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: interdiff

Comment 8

[Brendan Eich [:brendan]]

Attachment: best fix

Interdiff in a sec if needed.

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 337800 [details] [diff] [review]
interdiff

bugzilla interdiff can handle the "fixed fix" vs. "best fix" pair.

Rogue blank line before } while (0); at bottom of JSOP_SETNAME/SETPROP case, fixed for checkin in my tree.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 337822 [details] [diff] [review]
best fix

I like it.

Comment 11

[Brendan Eich [:brendan]]

Fixed in tracemonkey:

http://hg.mozilla.org/tracemonkey/rev/95bc5b468ebe

mrbkap is about to sync with m-c. Yay!

/be

Comment 12

[Brendan Eich [:brendan]]

Fixed on m-c:

http://hg.mozilla.org/mozilla-central/rev/95bc5b468ebe

/be