Closed Bug 454039 Opened 13 years ago Closed 13 years ago

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


(Core :: JavaScript Engine, defect, P1)






(Reporter: brendan, Assigned: brendan)




(1 file, 3 obsolete files)

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.

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.

Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b1
Attached patch fix (obsolete) — Splinter Review
Leaving JSOP_SETNAME for the other bug to keep patches smaller.

Attachment #337399 - Flags: review?(mrbkap)
Attachment #337399 - Flags: review?(mrbkap) → review+
Comment on attachment 337399 [details] [diff] [review]

>+++ 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);
>     }
> #endif    

Super-nit: remove the trailing whitespace after the endif.
Backed out. Something broke -- see bug 453918 comment 15 (and 16).

Attached patch fixed fix (obsolete) — Splinter Review
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.

Attachment #337399 - Attachment is obsolete: true
Attachment #337799 - Flags: review?(mrbkap)
Attached patch interdiff (obsolete) — Splinter Review
Attachment #337799 - Flags: review?(mrbkap) → review+
Attached patch best fixSplinter Review
Interdiff in a sec if needed.

Attachment #337799 - Attachment is obsolete: true
Attachment #337822 - Flags: review?(mrbkap)
Comment on attachment 337800 [details] [diff] [review]

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.

Attachment #337800 - Attachment is obsolete: true
Comment on attachment 337822 [details] [diff] [review]
best fix

I like it.
Attachment #337822 - Flags: review?(mrbkap) → review+
Fixed in tracemonkey:

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

Fixed on m-c:

Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 454689
Flags: in-testsuite-
Flags: in-litmus-
Depends on: 457660
You need to log in before you can comment on or make changes to this bug.