454689 - TM: record_SetPropMiss mis-layered on top of record_SetPropHit

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

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

Comment 1

[Brendan Eich [:brendan]]

May dup this against prior bug 454590.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

- 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

Comment 3

[Brendan Eich [:brendan]]

(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

Comment 4

[Brendan Eich [:brendan]]

(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

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

What's the testcase that hits viewIndex?  It seems to be used in some Places code in our tree....

Comment 7

[Brendan Eich [:brendan]]

bz: see bug 454590 -- cmd-shift-H on Mac, something less discoverable for bookmarks sidebar.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 10

[Brendan Eich [:brendan]]

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

Comment 11

[Brendan Eich [:brendan]]

Fixed on m-c:

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

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

> cmd-shift-H on Mac

Yeah, certainly Places stuff.