Closed Bug 454689 Opened 12 years ago Closed 12 years ago

TM: record_SetPropMiss mis-layered on top of record_SetPropHit

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: brendan, Assigned: brendan)

References

Details

Attachments

(1 file)

Patch for bug 454039 was not right. record_SetPropMiss is called when sprop is in the (mutated, own) scope of obj, so the test to decide whether to emit a call to F_AddProperty won't choose to emit. Need a fix today, this bug is the place.

/be
Summary: TM: don't abort TraceRecorder::record_JSOP_SETPROP on cache miss → TM: record_SetPropMiss mis-layered on top of record_SetPropHit
May dup this against prior bug 454590.

/be
Attached patch proposed fixSplinter Review
- Match assert about unallocated slots being JSVAL_VOID in js_AddProperty FASTCALL builtin to same assert in js_AllocSlot.
- Better ifdef'ed macrology for TRACE_[12] and RECORD.
- Fix SetPropHit to emit the F_AddProperty builtin call after the kshape guard only if the cache entry shows a transition to a new shape on trace.
- Fix SetPropMiss to deal with cache fill nesting under a resolve, addProperty, or setter, by qualifing entry as a first-level kpc to sprop mapping.

/be
Attachment #338061 - Flags: review?(mrbkap)
(In reply to comment #2)
> - Fix SetPropMiss to deal with cache fill nesting under a resolve, addProperty,
> or setter, 

Scratch the first two, it could only be a JSPROP_SHARED setter that runs script. But that should deepAbort us. Something's not making sense, because I saw entry containing a different kpc (valid pc, for a JSOP_CALLPROP -- the entry->vword was a function object, so this was a branded scope fill).

/be
(In reply to comment #3)
> (In reply to comment #2)
> > - Fix SetPropMiss to deal with cache fill nesting under a resolve, addProperty,
> > or setter, 
> 
> Scratch the first two, it could only be a JSPROP_SHARED setter that runs
> script. But that should deepAbort us. Something's not making sense, because I
> saw entry containing a different kpc (valid pc, for a JSOP_CALLPROP -- the
> entry->vword was a function object, so this was a branded scope fill).

But this makes sense: the set was a "no-fill" (uncacheable) so entry contained data from a prior fill for a different pc and shape.

Too tired to think of this earlier, so I'll sleep on it. But this patch looks good, on second and much closer look. Blake, what do you think?

/be
BTW, the id both times I saw a no-fill condition was "viewIndex" -- ring any bells? The three

                    PCMETER(JS_PROPERTY_CACHE(cx).nofills++);

lines are in jsobj.cpp, for default (class) getProperty returning a value from js_GetProperty, and for JSPROP_SHARED properties in js_SetProperty. The former is uncacheable -- no sprop or slot at all. The latter seems cacheable to me, if the shared sprop is direct. The first nofills++ in js_SetProperty is for a shared proto-prop, but the second (near the end) is for a direct property. This may be worth caching.

But first, need to identify that "viewIndex" perpetrator.

/be
What's the testcase that hits viewIndex?  It seems to be used in some Places code in our tree....
bz: see bug 454590 -- cmd-shift-H on Mac, something less discoverable for bookmarks sidebar.

/be
Duplicate of this bug: 454852
Attachment #338061 - Flags: review?(mrbkap) → review+
Comment on attachment 338061 [details] [diff] [review]
proposed fix

>-#ifdef JS_TRACER
>-                            TRACE_2(SetPropHit, kshape, sprop);
>-#endif
>+                            TRACE_2(SetPropHit, entry, sprop);

This will break non-tracer builds because the #include "jstracer.h" above is #ifdef'd as well.

r=mrbkap with that fixed.
Fixed on TM:

http://hg.mozilla.org/tracemonkey/rev/78289eec6471

with #include "jstracer.h" unifdef'ed except in jsapi.c, where JS_THREADSAFE is in the mix.

/be
Fixed on m-c:

http://hg.mozilla.org/mozilla-central/rev/9b5762656489

/be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
> cmd-shift-H on Mac

Yeah, certainly Places stuff.
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.