Closed Bug 454039 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: brendan, Assigned: brendan)

References

Details

Attachments

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

/be
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
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b1
Attached patch fix (obsolete) — Splinter Review
Leaving JSOP_SETNAME for the other bug to keep patches smaller.

/be
Attachment #337399 - Flags: review?(mrbkap)
Attachment #337399 - Flags: review?(mrbkap) → review+
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.
Backed out. Something broke -- see bug 453918 comment 15 (and 16).

/be
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.

/be
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.

/be
Attachment #337799 - Attachment is obsolete: true
Attachment #337822 - Flags: review?(mrbkap)
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
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:

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

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

/be
Fixed on m-c:

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

/be
Status: ASSIGNED → RESOLVED
Closed: 12 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.