[ I have found the bug when testing a patch for bug 427798 ] The JSOP_SETPROP case in js_Interpret contains the following lines BEGIN_CASE(JSOP_SETPROP) ... do { ... if (JS_LIKELY(obj->map->ops->setProperty == js_SetProperty)) { ... if (entry->kpc == regs.pc && entry->kshape == kshape) { ... if (scope->shape == kshape) { ... if (scope->object == obj) { /* * Fastest path: the cached sprop is already * in scope. Just NATIVE_SET and break to get * out of the do-while(0). */ ... } else { scope = js_GetMutableScope(cx, obj); ^^^^^^^^^^^^^^^^^^^^^^ ... } if (sprop->parent == scope->lastProp && !SCOPE_HAD_MIDDLE_DELETE(scope) && SPROP_HAS_STUB_SETTER(sprop) && That is, the code unconditionally creates a scope for property that is in the cache even when the setter is not a stub one and as such may not require the scope. This hit the patch for bug 427798 as it uses in an essential way scope sharing between block object and its clone. brendan, mrbkap: can you have a look at this?

This is what I use for testing of the patch for bug 427798. Note the patch would never call js_GetMutableScope when the cached property is not in scope. Currently the code always does that but I assume that this is another bug since the cached property may exist somewhere on the prototype chain.

If we hit the "level 1" (pc x shape) indexed cache, we know the property should be direct once we know the setter is stub. In that case, we do need to get a scope for the object in case it is a newly created one, sharing its proto-scope. Taking, I'll patch. This should block. Thanks for finding it, Igor! /be

See bug 419803 where JSOP_INITPROP was patched for the same kind of bug. /be

diff -w version next. /be

Questions: 1. Compared with js_SetPropertyHelper, the latest patch does not check for shared properties with a stub setter to avoid creating a scope for them. Is it because such properties would never be cached? 2. Why a non-stub setter can not be optimized when its property in the cache and exists in the scope? This clearly happens for cloned block objects since JSOP_BINDNAME puts the property into the cache (with or without the patch from bug 427798). Also, since the patch makes the optimization work only for a stub setter, it should not use NATIVE_SET for properties in the scope.

This bug should block 1.9, since it blocks a blocker.

To Brendan: could you comment on the issues from the comment 6?

Sorry, I will have to reload this patch into my brain and reply. /be

Any progress here? This is blocking one of the big remaining leak blockers so it'd be great to have fixed for release.

Working on this today, will have a new patch tonight. /be

(In reply to comment #6) > Questions: > > 1. Compared with js_SetPropertyHelper, the latest patch does not check for > shared properties with a stub setter to avoid creating a scope for them. Is it > because such properties would never be cached? Yes, hence the nofills metering. > 2. Why a non-stub setter can not be optimized when its property in the cache > and exists in the scope? This clearly happens for cloned block objects since > JSOP_BINDNAME puts the property into the cache (with or without the patch from > bug 427798). Also, since the patch makes the optimization work only for a stub > setter, it should not use NATIVE_SET for properties in the scope. Good point, I should simply handle stub and non-stub setters so long as the latter have a valid slot. Patch soon. /be

(In reply to comment #12) > > 2. Why a non-stub setter can not be optimized when its property in the cache > > and exists in the scope? > Good point, I should simply handle stub and non-stub setters so long as the > latter have a valid slot. Patch soon. But why check for a valid slot? I do not see what can be wrong with calling a setter that exists in the scope even if it is a shared one. This will be exactly the situation with a patch for bug 427798.

Brendan, progress?

(In reply to comment #13) > (In reply to comment #12) > > > 2. Why a non-stub setter can not be optimized when its property in the cache > > > and exists in the scope? > > > Good point, I should simply handle stub and non-stub setters so long as the > > latter have a valid slot. Patch soon. > > But why check for a valid slot? I do not see what can be wrong with calling a > setter that exists in the scope even if it is a shared one. This will be > exactly the situation with a patch for bug 427798. Yeah, the comments talk about optimizing for common cases, requiring extra guard checks that fail over to cache miss. That's silly if the cache miss code then fills the cache for the shared (slot-less) property. I will remove the slot-full hazards from JSOP_SET{NAME,PROP}. For JSOP_INITPROP the direct object is known to be an Object instance, so slot-full can be asserted. /be

Putting up for review, I am still testing but this is readable (I didn't change control flow in a way that re-indented otherwise untouched lines). /be

Passes suite (bc, js1_5/Regress/regress-169559.js seems to give false positives now and then -- true?). MochiTesting tonight. /be

Comment on attachment 317640 [details] [diff] [review] proposed fix, v2 > > if (scope->object == obj) { > /* > * Fastest path: the cached sprop is already > * in scope. Just NATIVE_SET and break to get > * out of the do-while(0). > */ > if (sprop == scope->lastProp || >@@ -4369,20 +4370,17 @@ interrupt: > } else { > scope = js_GetMutableScope(cx, obj); This still creates the scope for shared properties.

Sorry, was worrying about avoiding regressions, forgot the main fix! /be

Let's try that again... /be

(In reply to comment #17) > Passes suite (bc, js1_5/Regress/regress-169559.js seems to give false positives It used to before the recent performance work but it has been pretty stable for me lately and has stopped failing. If it is starting to fail again, I would call that a regression.

Comment on attachment 317684 [details] [diff] [review] proposed fix, v4 >+ /* >+ * Handle setting a shared prototype property >+ * before calling js_GetMutableScope, which is >+ * unnecessary in this case. See the (attrs & >+ * JSPROP_SHARED) case in js_SetPropertyHelper >+ * in jsobj.c. >+ */ >+ if (sprop->attrs & JSPROP_SHARED) { >+ JS_ASSERT(sprop->slot == >+ SPROP_INVALID_SLOT); >+ JS_ASSERT(!SPROP_HAS_STUB_SETTER(sprop) || >+ (sprop->attrs & JSPROP_GETTER)); The code in js_SetPropertyHelper contains: if (attrs & JSPROP_SHARED) { if (SPROP_HAS_STUB_SETTER(sprop) && !(sprop->attrs & JSPROP_GETTER)) { if (entryp) { PCMETER(JS_PROPERTY_CACHE(cx).nofills++); *entryp = NULL; } return JS_TRUE; } if (entryp) { js_FillPropertyCache(cx, obj, type, 0, protoIndex, pobj, sprop, entryp); } return SPROP_SET(cx, sprop, obj, pobj, vp); } That is, the code allows for SPROP_HAS_STUB_SETTER(sprop) && !(sprop->attrs & JSPROP_GETTER) So why the property cache asserts agains that? AFAICS a property of a prototype of a global object with stub getter/setter with shared attribute would still be cached in JSOP_BINDNAME.

(In reply to comment #22) > (From update of attachment 317684 [details] [diff] [review]) > >+ /* > >+ * Handle setting a shared prototype property > >+ * before calling js_GetMutableScope, which is > >+ * unnecessary in this case. See the (attrs & > >+ * JSPROP_SHARED) case in js_SetPropertyHelper > >+ * in jsobj.c. > >+ */ > >+ if (sprop->attrs & JSPROP_SHARED) { > >+ JS_ASSERT(sprop->slot == > >+ SPROP_INVALID_SLOT); > >+ JS_ASSERT(!SPROP_HAS_STUB_SETTER(sprop) || > >+ (sprop->attrs & JSPROP_GETTER)); > > The code in js_SetPropertyHelper contains: > > if (attrs & JSPROP_SHARED) { > if (SPROP_HAS_STUB_SETTER(sprop) && > !(sprop->attrs & JSPROP_GETTER)) { > if (entryp) { > PCMETER(JS_PROPERTY_CACHE(cx).nofills++); Notice no fill here, so it's impossible to have a first-level (kpc * kshape) hit at the JSOP_SETPROP or JSOP_SETNAME. > *entryp = NULL; > } > return JS_TRUE; > } > > if (entryp) { > js_FillPropertyCache(cx, obj, type, 0, protoIndex, > pobj, sprop, entryp); So fill here only if !(SPROP_HAS_STUB_SETTER(sprop) && !(sprop->attrs & JSPROP_GETTER)) which by De Morgan's theorem is !SPROP_HAS_STUB_SETTER(sprop) || (sprop->attrs & JSPROP_GETTER). The latter term in the || causes us to SPROP_SET: > } > return SPROP_SET(cx, sprop, obj, pobj, vp); just in order to get the JSMSG_GETTER_ONLY error. > } > > That is, the code allows for > > SPROP_HAS_STUB_SETTER(sprop) && !(sprop->attrs & JSPROP_GETTER) ... but does not fill the first-level cache in that case. And we know in the JSOP_SETNAME/JSOP_SETPROP code in js_Interpret that we're hitting at first-level only. > So why the property cache asserts agains that? AFAICS a property of a prototype > of a global object with stub getter/setter with shared attribute would still be > cached in JSOP_BINDNAME. Not for a different pc. /be

(In reply to comment #23) > > So why the property cache asserts agains that? AFAICS a property of a prototype > > of a global object with stub getter/setter with shared attribute would still be > > cached in JSOP_BINDNAME. > > Not for a different pc. Hm, pc -> cash index mapping is a hash. Thus a collision between JSOP_BINDNAME/JSOP_SETNAME pc is possible especially given that JSOP_BINDNAME can be an arbitrary distance from JSOP_SETNAME. Since js_FindPropertyHelper does fill the cache without checking for getters/setters, the assert is wrong. This also suggests to use in JSOP_BINDNAME the pc of JSOP_SETNAME to avoid cache refils, but this is for another bug.

(In reply to comment #24) > Hm, pc -> cash index mapping is a hash. Thus a collision between > JSOP_BINDNAME/JSOP_SETNAME pc is possible especially given that JSOP_BINDNAME > can be an arbitrary distance from JSOP_SETNAME. Not likely for the same kshape, but sure: the cache is direct-mapped, there are collisions. Very few -- turn on JS_PROPERTY_CACHE_METERING and look for *recycles numbers. Direct mapped means last filler wins. > Since js_FindPropertyHelper > does fill the cache without checking for getters/setters, the assert is wrong. No. The cache hash probe is checked for exact pc and shape match. > This also suggests to use in JSOP_BINDNAME the pc of JSOP_SETNAME to avoid > cache refils, but this is for another bug. Fill two entries? Only if we know the shape won't change. ECMA-262 requires binding before evaluating the RHS of assignment, which could mutate arbitrarily. /be

Don't see an approval request here. Is anything else needed to land this?

I'm adding some assertions. /be

Igor, feel free to ask questions. This has only comment, assertion, and blank line changes from the last patch. /be

Last patch was a non-starter for MochiTests. This passes js/tests and MochiTests. diff -w next, more comments after sleep. /be

Comment on attachment 317858 [details] [diff] [review] proposed fix, v5 >+ obj2 = scope->object; >+ if (sprop->attrs & JSPROP_SHARED) { ... >+ if (!SPROP_SET(cx, sprop, obj, obj2, &rval)) { >+ JS_UNLOCK_SCOPE(cx, scope); I should have seen this before: SPROP_SET requires an unlocked scope.

It would be easy to unlock first, but I'm not happy introducing JSPROP_SHARED special case code into the "fast path" in JSOP_SETNAME/JSOP_SETPROP at this point. It may or may not cater to the common case (it doesn't for SunSpider, and the blocked let-binding bug will go through JSOP_SETLOCAL most of the time, using JSOP_SETNAME only for outer let-bound names where closures are being used). So instead of adding code, this patch removes the code that fills JSPROP_SHARED properties into the cache. It passes js and Mochi tests. /be

Comment on attachment 317889 [details] [diff] [review] safer fix for 1.9 Ideally comments about non-caching of shared properties would be nice, but even as it is the patch does the right thing. It emphasizes on caching things that in practice benefit from caching without adding source complexity for supporting esoteric cases.

Comment on attachment 317901 [details] [diff] [review] with comment >- /* Don't clone a shared prototype property. */ >+ /* >+ * Don't clone a shared prototype property. Don't fill it in the >+ * property cache either, since the JSOP_SETPROP/JSOP_SETNAME code >+ * in js_Interpret does not handle shared or prototype properties. >+ * Shared prototype properties require more hit qualification than >+ * the fast-path code for those ops, which is targeted on direct, >+ * slot-based properties. >+ */ Even nicer would be to have an assert in jsinter.c that refer to the comments.

Comment on attachment 317901 [details] [diff] [review] with comment The js_SetPropertyHelper patch affects only prototype properties. Directly set shared props are still cached. /be

This tests well too. The added !(attrs & JSPROP_SHARED) test guarding the fill at the bottom of js_SetPropertyHelper avoids putting a direct shared property in the first-level cache, since the jsinterp.c "Faster path" code that tries to extend a scope not yet containing the property will fail for want of a valid slot (if the scope already has the shared property, then the "Fastest path" code can set it). The only way to cause a direct JSPROP_SHARED-attributed property to be created under js_SetPropertyHelper is via the old JSCLASS_SHARE_ALL_PROPERTIES flag, but a direct set on a prototype whose property was pre-defined (e.g. by XPConnect) with JSPROP_SHARED would also go through this case, on first cache miss or from an API "Set" call. There's more to do for shared property sets hitting the cache, but later. /be

(In reply to comment #37) > The only way to cause a direct JSPROP_SHARED-attributed property to be created > under js_SetPropertyHelper is via the old JSCLASS_SHARE_ALL_PROPERTIES flag, > but a direct set on a prototype whose property was pre-defined (e.g. by > XPConnect) with JSPROP_SHARED would also go through this case, on first cache > miss or from an API "Set" call. Of course, the API call into js_SetProperty, then js_SetPropertyHelper, passes null entryp, so no fill is even attempted there. Just wanted to follow up to add clarity here. /be

Comment on attachment 317938 [details] [diff] [review] forgot nofills metering a=mconnor on behalf of 1.9 drivers

Fixed: js/src/jsinterp.c 3.492 js/src/jsobj.c 3.470 /be