487204 - js_FillPropertyCache is called with garbage-collected pobj

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

The current code, when calls js_FillPropertyCache for properties found on prototype chains, does not detect that a setter or getter could have deleted the prototype and GC-ed it. This would cause js_FillPropertyCache to access GC-ed objects. Here is an example that demonstrates this:

@watson~/m/tm/js/src> cat ~/s/x1.js
var obj = { __proto__: { get a() { this.__proto__ = null; gc(); return 0; }}};

obj.a;

@watson~/m/tm/js/src> ~/build/js32.tm.dbg/js ~/s/x1.js
before 20480, after 20480, break 08262000
Segmentation fault

This bug exists on 1.9.* branches and may (or may not) be related to the bug 484042.

Comment 1

[Igor Bukanov]

I wonder why js_FillPropertyCache is called after the code calls js_NativeGet or js_NativeSet that could have invalidate the property cache and collect objects?

Comment 2

[Igor Bukanov]

Another problem with the current code comes from patterns like

js_NativeGet(cx, obj, obj2, sprop, vp)
OBJ_DROP_PROPERTY(cx, obj2, prop);

With examples from the comment 0 this is unsound as obj2 can be GC-ed in js_NativeGet.

Comment 3

[Igor Bukanov]

Attachment: v1

Here is a minimal fix that should require minimal 1.9.0 porting efforts. It does 2 things:

1. Root pobj in js_NativeGet. This missing rooting exists on 181 branch as well. It is just sad that I have missed the need for that after all those changes to root sprop in js_NativeGet some time ago. 

2. Change js_FillPropertyCache callers to invoke the function before any getter or setter has a chance to run.

I will do some testing before asking for a review.

Comment 4

[Igor Bukanov]

Comment on attachment 371452 [details] [diff] [review]
v1

The patch passes shell tests and try server.

Comment 5

[Brendan Eich [:brendan]]

We want to remove settable proto (resettable, I should say: initializers should be able to preset), in which happy day the code added here should be removed.

js_FillPropertyCache is called after because it seemed best not to fill after an error from the getter.

Did you consider avoiding tvr2 by extending the way propertyRemovals is bumped and testing it earlier?

/be

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> js_FillPropertyCache is called after because it seemed best not to fill after
> an error from the getter.

Such fill-after-getter is wrong since the getter can run GC invalidating all shape values.

> 
> Did you consider avoiding tvr2 by extending the way propertyRemovals is bumped
> and testing it earlier?

When propertyRemovals is bumped, the code does not know if it was bumped due to removal of sprop or some other property on this or another object. So this would not help the GC safety of sprop and pobj.

To optimize the rooting I considered adding a specialized TVR to root both sprop and pobj in one step. But that requires quite a few extra lines of code and I wanted to have a minimal patch for simpler porting to branches.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (In reply to comment #5)
> > js_FillPropertyCache is called after because it seemed best not to fill after
> > an error from the getter.
> 
> Such fill-after-getter is wrong since the getter can run GC invalidating all
> shape values.

We want to leave live shape values alone during GC if we can, but that is a bug for another day.

But the shapes will not be invalid in the objects, and js_FillPropertyCache uses these regenerated shapes when filling to prime the cache for a future hit assuming no GC between fill and future hit. So I do not see how GC invalidating the shapes at the time the getter is called breaks anything used to fill the cache for future hits after the getter unwinds.

The GC safety bug is separate.

> When propertyRemovals is bumped, the code does not know if it was bumped due to
> removal of sprop or some other property on this or another object.

The idea (rename it getterHazards or some such) is that any change from sample means a (rare) bad thing happened, and js_NativeGet should cope. But this does not help its caller avoid using pobj/obj2, of course. We'd have to extend the API to js_NativeGet somehow.

/be

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> But the shapes will not be invalid in the objects, and js_FillPropertyCache
> uses these regenerated shapes when filling to prime the cache for a future hit
> assuming no GC between fill and future hit.

This is not true. js_FillPropertyCache uses the passed kshape when the property comes from the prototype. It does not read the regenerated shape. If the function is called after the getter, that shape may not match the real value of the shape.

> The idea (rename it getterHazards or some such) is that any change from sample
> means a (rare) bad thing happened, and js_NativeGet should cope.

SpiderMonkey has not yet include a function called ungc(). Until such function would be provided js_NativeGet could not cope with sporadically GC-ed property objects.

>  We'd have to extend the API to js_NativeGet somehow.

Just consider why js_NativeGet needs to root both sprop and pobj. This is just to execute LOCKED_OBJ_SET_SLOT(pobj, slot, *vp):

    JS_LOCK_SCOPE(cx, scope);
    JS_ASSERT(scope->object == pobj);
    if (SLOT_IN_SCOPE(slot, scope) &&
        (JS_LIKELY(cx->runtime->propertyRemovals == sample) ||
         SCOPE_GET_PROPERTY(scope, sprop->id) == sprop)) {
        LOCKED_OBJ_SET_SLOT(pobj, slot, *vp);
    }

I could understand the compatibility reasoning why this SET_SLOT may be necessary when pobj == obj. But I fail to see why for a getter comming from a prototype the code needs to store its result inside the pobj and not in the object itself. As the example shows, pobj may even no longer be a prototype for obj!

So if anything, the API should be changed so remove that need for LOCKED_OBJ_SET_SLOT. After that one no longer would needs to lock the scope second time or root pobj or scope.

Comment 9

[Igor Bukanov]

As a consequence of that caching-aftter-(getter|setter) js_FillPropertyCache needs to explicitly check if the sprop is still in the scope:

    /*                                                                                                                                                             
     * Check for fill from js_SetPropertyHelper where the setter removed sprop                                                                                     
     * from pobj's scope (via unwatch or delete, e.g.).                                                                                                            
     */
    scope = OBJ_SCOPE(pobj);
    JS_ASSERT(scope->object == pobj);
    if (!SCOPE_HAS_PROPERTY(scope, sprop)) {
        PCMETER(cache->oddfills++);
        *entryp = NULL;
        return;
    }

This de-optimizes the fill for most getters and setters that typically do not remove any properties from the scope. Another problem is the need to re-calculate protoIndex.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #7)
> > But the shapes will not be invalid in the objects, and js_FillPropertyCache
> > uses these regenerated shapes when filling to prime the cache for a future hit
> > assuming no GC between fill and future hit.
> 
> This is not true. js_FillPropertyCache uses the passed kshape when the property
> comes from the prototype. It does not read the regenerated shape. If the
> function is called after the getter, that shape may not match the real value of
> the shape.

Good point -- but bear with me here, if we have a way to detect removals/clear-scope/gc then we could reload kshape too. I'm wrong about the current code but thinking about a possible future that avoids too much manual rooting for a rare case (which in part -- __proto__ setting -- will go away, I think in the next release).

> > The idea (rename it getterHazards or some such) is that any change from sample
> > means a (rare) bad thing happened, and js_NativeGet should cope.
> 
> SpiderMonkey has not yet include a function called ungc(). Until such function
> would be provided js_NativeGet could not cope with sporadically GC-ed property
> objects.

I'm suggesting the API to js_NativeGet change such that the caller can know to bail (not fill, not unlock a dangling pobj). We have similar optimizations and asymmetric calling conventions, some recently engineered by you IIRC. Pessimal rooting and locking for symmetry's sake is best in public APIs, but we are deep in the belly of the beast here.

> >  We'd have to extend the API to js_NativeGet somehow.
> 
> Just consider why js_NativeGet needs to root both sprop and pobj. This is just
> to execute LOCKED_OBJ_SET_SLOT(pobj, slot, *vp):
> 
>     JS_LOCK_SCOPE(cx, scope);
>     JS_ASSERT(scope->object == pobj);
>     if (SLOT_IN_SCOPE(slot, scope) &&
>         (JS_LIKELY(cx->runtime->propertyRemovals == sample) ||
>          SCOPE_GET_PROPERTY(scope, sprop->id) == sprop)) {
>         LOCKED_OBJ_SET_SLOT(pobj, slot, *vp);
>     }
> 
> I could understand the compatibility reasoning why this SET_SLOT may be
> necessary when pobj == obj.

That's the only good reason. The rest is ancient history.

> But I fail to see why for a getter coming from a
> prototype the code needs to store its result inside the pobj and not in the
> object itself.

Indeed this is an old design flaw, which led to entrainment and pigeon-hole problems in proto-slots, which begat JSPROP_SHARED. We should try to get rid of it now, if we can -- even in a patch for this bug.

> As the example shows, pobj may even no longer be a prototype for
> obj!

That is only due to settable __proto__. You're right about that particular hazard but we want to remove it, so we should bias away from it.

> So if anything, the API should be changed so remove that need for
> LOCKED_OBJ_SET_SLOT. After that one no longer would needs to lock the scope
> second time or root pobj or scope.

Agreed!

/be

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Indeed this is an old design flaw, which led to entrainment and pigeon-hole
> problems in proto-slots, which begat JSPROP_SHARED. We should try to get rid of
> it now, if we can -- even in a patch for this bug.

This bug exists on 1.9.0 (and on 1.8.1). So before doing any optimization that could break things I would like to have a minimal patch that could land with minimal porting efforts on branches.

Given that, consider that if we can drop that slot assignment for the pobj != obj case, then there would be no need to lock pobj the second time (assuming that the cache fill is done before calling the getter as the current patch suggests). It would remove the need to root pobj.

And if we can remove slot's assignment for pobj == obj, then rooting of sprop can also be removed. But this is for another bug.

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> This bug exists on 1.9.0 (and on 1.8.1). So before doing any optimization that
> could break things I would like to have a minimal patch that could land with
> minimal porting efforts on branches.

On the second thought, given how bad is this bug (call through a function pointer on GC-ed object in OBJ_DROP_PROPERTY), it could be dangerous to leak any information about it. A minimal patch for a restricted bug landing on the trunk could in theory provide quite a strong hint where to look for a bug on 1.9.0. An optimization patch for an open 1.9.1 bug would reveal nothing. 

But I could be too paranoid about this.

Comment 13

[Brendan Eich [:brendan]]

Between paranoia and desire for optimization to stay competitive, I am hoping you will apply some of that patented Igor case analysis goodness, tout suite!

/be

Comment 14

[Igor Bukanov]

Attachment: v2

The new version of the patch removes extra validating code from js_FillPropertyCache that SCOPE_HAS_PROPERTY(scope, sprop). It is possible since now js_FillPropertyCache is always called before the object is unlocked and any setter or getter are run. It also fixes the premature shape access issues from the bug 487846 - that was trivial to do since code always passes OBJ_SHAPE(obj) for kshape argument where obj is the second argument to the function.

These two changes are more then enough to offset any slowdown coming from the extra rooting in js_NativeGet. But there are more opportunities to optimize even without semantic changes proposed at the end of the comment 8. 

The slot writing code in js_Native(Get|Set) is only need to run for non-shared getters. Thus for shared getters the code can already skip the need to lock the object again after the getter returns. But this implies changing js_Native(Get|Set) semantic to always return unlocked object. It would make the patch much bigger and not suitable for 1.9.0 porting. Hence I give this patch for references.

Comment 15

[Igor Bukanov]

Attachment: v3 (base)

The new version fixes bogus changes for kshape calculation in  js_FillPropertyCache.

Comment 16

[Igor Bukanov]

Comment on attachment 372204 [details] [diff] [review]
v3 (base)

Asking for a review of this part - other changes to skip extra locking for shared getters or setters require that properties are cached before calling them.

Comment 17

[Igor Bukanov]

Attachment: v3 (avoiding extra locking)

This new patch skips the need to take the scope look the second time for shared getters and setters. This changes the semantic of js_Native(Get|Set) to always unlock the scope on return.

Comment 18

[Igor Bukanov]

Comment on attachment 372204 [details] [diff] [review]
v3 (base)

clearing accidentally set obsolete flag

Comment 19

[Brendan Eich [:brendan]]

Caching based on the before-SetPropertyHelper key-object shape is critical for performance. Have you regression tested, e.g., shell sunspider? Using the post-set shape means a guaranteed miss when the set or define property operation is predictably evolving an object from fewer to more properties.

/be

Comment 20

[Igor Bukanov]

(In reply to comment #19)
> Caching based on the before-SetPropertyHelper key-object shape is critical for
> performance. Have you regression tested, e.g., shell sunspider?

I observed no differences with sunspider or with synthetic benchmarks.

> Using the
> post-set shape means a guaranteed miss when the set or define property
> operation is predictably evolving an object from fewer to more properties.

You do not mean post-set here, but rather post-adding-sprop-to-scope, right? This is very different from caching based on the sprop after getter/setter execution. I see the need for the former (but only for the case when the property is really added, not when set has discovered it) but the later is a bug.

Comment 21

[Igor Bukanov]

I suspect the reason the benchmarks has not detected any deoptimization with the patch is that those predictive shape optimizations is done is js_Interpret. So the patch cannot affect them.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #20)
> (In reply to comment #19)
> > Caching based on the before-SetPropertyHelper key-object shape is critical for
> > performance. Have you regression tested, e.g., shell sunspider?
> 
> I observed no differences with sunspider or with synthetic benchmarks.

Real work must happen in js_Interpret for the performance-critical cases. My memory is unclear of 3.0-era testing, but that's where the "add" (not set per se as you point out) case is optimized.

> > Using the
> > post-set shape means a guaranteed miss when the set or define property
> > operation is predictably evolving an object from fewer to more properties.
> 
> You do not mean post-set here, but rather post-adding-sprop-to-scope, right?

Right, although the setter for an already-added property might predictably alter the shape too. But this seems not to be perf-critical, and the worst case is exactly this bug, so we should fix the safety problem first and worry about any lingering perf issue second.

> This is very different from caching based on the sprop after getter/setter
> execution. I see the need for the former (but only for the case when the
> property is really added, not when set has discovered it) but the later is a
> bug.

It still seems desirable to avoid re-sampling kshape when there is no hazard. It does not seem like that much code. Thoughts?

/be

Comment 23

[Igor Bukanov]

(In reply to comment #19)
> Using the
> post-set shape means a guaranteed miss when the set or define property
> operation is predictably evolving an object from fewer to more properties.

This I do not understand. The current code checks the real shape of the object when doing hash-compare. If js_FillPropertyCache writes the shape before the property was added, how that can match the shape on the following shape test in js_Interpret?

Comment 24

[Igor Bukanov]

I just checked the following script with the debugger.

(function() {
  var obj = {};
  for (var i = 0; i != 2; ++i)
    obj.a = 1;
})()

Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a. This is precisely because the code stores the wrong value of obj's shape. *With* the patch on the second run the cache was hit.  So the patch makes things better, not worse.

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #23)
> (In reply to comment #19)
> > Using the
> > post-set shape means a guaranteed miss when the set or define property
> > operation is predictably evolving an object from fewer to more properties.
> 
> This I do not understand. The current code checks the real shape of the object
> when doing hash-compare. If js_FillPropertyCache writes the shape before the
> property was added, how that can match the shape on the following shape test in
> js_Interpret?

Different object, same progression. The key includes the pc, you don't see the same pc add the same property to the same object again. E.g.,

function fast()   { var o = {p:1, q:2, r:3} ... }
function fast2(o) { o.p = 1; o.q = 2; o.r = 3; }

/be

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #24)
> I just checked the following script with the debugger.
> 
> (function() {
>   var obj = {};
>   for (var i = 0; i != 2; ++i)
>     obj.a = 1;
> })()
> 
> Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a.

First time is a miss no matter what.

After the second time, no more misses -- right? (Loop till 10 instead of 2.)

Cache is for predictable regimes such as adding properties explicitly, or loop after it has warmed up.

/be

Comment 27

[Igor Bukanov]

(In reply to comment #25)

> Different object, same progression. The key includes the pc, you don't see the
> same pc add the same property to the same object again. E.g.,
> 
> function fast()   { var o = {p:1, q:2, r:3} ... }
> function fast2(o) { o.p = 1; o.q = 2; o.r = 3; }

I just checked in the debugger both these cases without the patch. They both show the cache miss 3 times. There were no cache hits. This is no surprising since the code does that special adding of properties without changing the shapes only when pc stays the same. This picture remains the same with the patch.

>> Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a.

> First time is a miss no matter what.

> After the second time, no more misses -- right? (Loop till 10 instead of 2.)
 
Yes, it no longer misses. The question is why it should miss the second time?

Comment 28

[Brendan Eich [:brendan]]

(In reply to comment #27)
> (In reply to comment #25)
> 
> > Different object, same progression. The key includes the pc, you don't see the
> > same pc add the same property to the same object again. E.g.,
> > 
> > function fast()   { var o = {p:1, q:2, r:3} ... }
> > function fast2(o) { o.p = 1; o.q = 2; o.r = 3; }
> 
> I just checked in the debugger both these cases without the patch. They both
> show the cache miss 3 times. There were no cache hits.

Calling which function how many times?

Of course you have to call fast twice, or fast2 twice, to get hits.

> This is no surprising
> since the code does that special adding of properties without changing the
> shapes only when pc stays the same. This picture remains the same with the
> patch.

Right.

> >> Without the patch it misses the cache twice in JSOP_SETPROP when doing obj.a.
> 
> > First time is a miss no matter what.
> 
> > After the second time, no more misses -- right? (Loop till 10 instead of 2.)
> 
> Yes, it no longer misses. The question is why it should miss the second time?

First is a miss because nothing filled for the "callsite". For this miss we fill using the before-set kshape (without your patch). That is a guaranteed miss for this kind of code the second time around the loop, as you noted. This is a bug. Can we have best of both worlds with the fix you attached? Or do we need more logic, to distinguish the two cases and handle each only when it is at hand?

If your patch shows zero regressions, then it means the interpreter handles all the fast/fast2 type cases, and the jsobj.cpp code handles the obj.a = 1 in a loop cases.

/be

Comment 29

[Igor Bukanov]

(In reply to comment #28)
> Calling which function how many times?

When the function is called the second times, then it works with or without the patch in the same way. The reason for this is that the interpreter would optimize this cases adding the properties *without* changing the shapes. 

There is zero prediction here. It is just that interpreter knows when it is safe to inline js_SetPropertyHelper and add a property without changing the shape.

> First is a miss because nothing filled for the "callsite". For this miss we
> fill using the before-set kshape (without your patch). That is a guaranteed
> miss for this kind of code the second time around the loop, as you noted. This
> is a bug. Can we have best of both worlds with the fix you attached?

Yes, this is so with the patch. As before, the interpreter takes care about known cases and does not update the shape so the cache entry will be hit again. Plus with the patch js_FillPropertyCache always writes to the cache the value that ensure the hit when the bytecode is executed again.

Comment 30

[Brendan Eich [:brendan]]

(In reply to comment #29)
> There is zero prediction here. It is just that interpreter knows when it is
> safe to inline js_SetPropertyHelper and add a property without changing the
> shape.

The prediction is based on pc and what happened last time (last call to fast or fast2); at least that is what I meant by "prediction". It's history-based, aka cache-based.

> Yes, this is so with the patch. As before, the interpreter takes care about
> known cases and does not update the shape so the cache entry will be hit again.
> Plus with the patch js_FillPropertyCache always writes to the cache the value
> that ensure the hit when the bytecode is executed again.

Ok, sold. I think I had some low-level misses from the Firefox 3.0 daze that this explains. I will review shortly.

Combined patch worth it for branch too?

/be

Comment 31

[Igor Bukanov]

I was very wrong when claiming that the patch does not deoptimizes the cases of adding new property. It does precisely that! If another object follows the same property addition path as the cached one, then for caching to work it must store the shape before adding the last property.

When I was debugging those cases with the patch I was too quick to assume that everything was right. Very bad!

Comment 32

[Brendan Eich [:brendan]]

Whew, that accords with my original thinking. I'm preoccupied with upvar fallout, so glad you are checking my early claims here!

/be

Comment 33

[Igor Bukanov]

Attachment: v4 (base)

The new patch makes sure that mutators cache based on the value of the shape prior the addition of the property not to deoptimize object initializers. But the patch reads the shape after any resolve hooks and js_AddScopeProperty are run. This way it never stores GC-ed shape number in the cache.

Comment 34

[Igor Bukanov]

Attachment: v4 (avoiding extra locking)

Here is an updated patch to avoid extra locking after running a shared getter or setter. It could be ported to 1.9.0 for an extra performance, but this is not required.

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 372618 [details] [diff] [review]
v4 (base)

>+     * Check for overdeep scope and prototype chain. Because resolve hooks
>+     * that js_LookupPropertyWithFlags runs can change the prototype chain
>+     * using JS_SetPrototype, we have to validate protoIndex if it is
>+     * non-zero. If it is zero, then we know from the fact that obj == pobj,
>+     * that protoIndex is invariant.

No need for , at end of next-to-last comment line.

>+                 * Here on the first iteration JSOP_SETPROP fills the cache
>+                 * with the shape of newly created object, no the shape after

s/no the shape/not the shape/

>+                 * obj.x is assigned. That mismatches obj's shape on the
>+                 * second iteration. Note that on third and the following
>+                 * iterations the cache will be hit since the shape no longer
>+                 * mutates.
>+                 */
>+                JS_ASSERT(scope->object == obj);
>+                JS_ASSERT(sprop == scope->lastProp);
>+                if (sprop->parent) {
>+                    kshape = sprop->parent->shape;
>+                } else {
>+                    JSObject *proto = STOBJ_GET_PROTO(obj);
>+                    if (proto && OBJ_IS_NATIVE(proto))
>+                        kshape = OBJ_SHAPE(proto);
>+                }
>+            }

What if the scope is branded? Its shape then evolves independently from scope->lastProp->shape, but uniquely with each added property. See SCOPE_EXTEND_SHAPE and its uses.

>+/*
>+ * If addedSprop is not null, it must be initialized to false. On exit it will
>+ * be set to true when the property was added to the scope.

s/when the property was added/if this call added the property/

/be

Comment 36

[Brendan Eich [:brendan]]

Comment on attachment 372621 [details] [diff] [review]
v4 (avoiding extra locking)

> JSBool
> js_NativeGet(JSContext *cx, JSObject *obj, JSObject *pobj,
 . . .
>+    *vp = LOCKED_OBJ_GET_SLOT(pobj, slot);
>+    if (!SPROP_HAS_STUB_GETTER(sprop)) {
>+        /*
>+         * For API compatibility we must set the slot with getter's result so

"the getter's result", and commas after "compatibility" and "result".

>+         * we have to make sure that it is still valid after we run the

s/have to/must/

>+         * getter.
>+         */
>+        int32 sample = cx->runtime->propertyRemovals;
>+        JSTempValueRooter tvr, tvr2;

Could we have a followup bug on breaking the API and otherwise optimizing more here? I believe we can get rid of tvr2 at least, with similar/combined approach as is used for propertyRemovals.

>+ * This function must be called with pobj's scope locked. On return that scope
>+ * is always unlocked. After function returns the caller must not access pobj

"After this function returns" and comma after "returns".

>+ * or sprop unless it knows that sprop's getter is a stub.
>  */
> extern JSBool
> js_NativeGet(JSContext *cx, JSObject *obj, JSObject *pobj,
>              JSScopeProperty *sprop, jsval *vp);
> 
>+/*
>+ * This function must be called with obj's scope locked. On return that scope
>+ * is always unlocked. After function returns the caller must not access sprop

Ditto.

/be

Comment 37

[Brendan Eich [:brendan]]

Given size of first patch, I would prefer a combined patch for 1.9.0.x and newer branches/repos.

/be

Comment 38

[Igor Bukanov]

(In reply to comment #35)
> What if the scope is branded? Its shape then evolves independently from
> scope->lastProp->shape, but uniquely with each added property. See
> SCOPE_EXTEND_SHAPE and its uses.

The branded scopes are not set-optimized, right? I.e. the next time interpreter hits the PC that adds a property to branded scope the shape will always be different. Then it is better to cache on the shape after the property is added to gain efficiency with loop that creates a property on the first iteration and then only updates its value. 

I will update the patch to set addedSprop only if js_AddScopeProperty has evolved the scope predictably. Then the flag should be called differently, like predictablyAddedSprop or something.

Comment 39

[Brendan Eich [:brendan]]

Branding ideally ends after an object has been "set up" (methods, i.e. function valued properties, have been assigned -- some work remains here, see bug 471214).

It might be best to be concrete, and pass kshape into js_FillPropertyCache again, declaring it in js_{DefineNative,Set}Property, and passing it as out parameter to js_AddScopeProperty so it can be set in all successful return cases.

/be

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #39)
> Branding ideally ends after an object has been "set up"

I meant "shape evolution", not "branding" -- the SCOPE_BRANDED flag must remain set so that GC_WRITE_BARRIER can do its thing and protect the cache and traces from using stale "method values".

/be

Comment 41

[Igor Bukanov]

(In reply to comment #39)
> It might be best to be concrete, and pass kshape into js_FillPropertyCache
> again, declaring it in js_{DefineNative,Set}Property, and passing it as out
> parameter to js_AddScopeProperty so it can be set in all successful return
> cases.

The problem is that implies a few extra checks due to shape-invalidating js_GenerateShape. That may happen in js_AddScopeProperty and also can happen in js_FillPropertyCache itself during SCOPE_MAKE_UNIQUE_SHAPE for the branded scope (note the bug in the current code that updates kshape only if OBJ_SCOPE(obj) == scope - this is not enough).

So passing kshape parameter is not particularly useful and just invites for hazards due to hidden shape-invalidating js_GenerateShape.

Comment 42

[Igor Bukanov]

(In reply to comment #41)

> The problem is that implies a few extra checks due to shape-invalidating
> js_GenerateShape. That may happen in js_AddScopeProperty and also can happen in
> js_FillPropertyCache itself during SCOPE_MAKE_UNIQUE_SHAPE for the branded
> scope

Which also implies that js_FillPropertyCache must check if the cache was not disabled after that SCOPE_MAKE_UNIQUE_SHAPE. One more bug to fix.

Comment 43

[Brendan Eich [:brendan]]

Comment on attachment 372618 [details] [diff] [review]
v4 (base)

>     /*
>-     * Check for fill from js_SetPropertyHelper where the setter removed sprop
>-     * from pobj's scope (via unwatch or delete, e.g.).
>-     */
>-    scope = OBJ_SCOPE(pobj);
>-    JS_ASSERT(scope->object == pobj);
>-    if (!SCOPE_HAS_PROPERTY(scope, sprop)) {
>-        PCMETER(cache->oddfills++);
>-        *entryp = NULL;
>-        return;
>-    }
>-

This case could happen, is it safe to replace it with an assertion? If so, please remove the oddfills meter.

/be

Comment 44

[Igor Bukanov]

(In reply to comment #43)
> >     /*
> >-     * Check for fill from js_SetPropertyHelper where the setter removed sprop
> >-     * from pobj's scope (via unwatch or delete, e.g.).
> >-     */
> 
> This case could happen, is it safe to replace it with an assertion? If so,
> please remove the oddfills meter.

I will remove oddfills - with the patch the js_FillPropertyCache should always be called before any getter or setter invocation.

Comment 45

[Igor Bukanov]

(In reply to comment #36)
> >+ * This function must be called with pobj's scope locked. On return that scope
> >+ * is always unlocked. After function returns the caller must not access pobj
> 
> "After this function returns" and comma after "returns".

To Brendan:

When I fixed the comments as suggested, the whole comment block became aligned much better improving readability. This happens noticeably more often than the change in the opposite direction when the alignment worsens. So how do you do that??? I don't believe that you count the number of letters in the text ;) !

Comment 46

[Brendan Eich [:brendan]]

It's my mutant gift ;-).

/be

Comment 47

[Igor Bukanov]

Attachment: v5 (base)

The new version cache on the old shape only for predictive scope adds. It also fixes js_FillPropertyCache to exit when SCOPE_MAKE_UNIQUE_SHAPE for branded scopes disable the cache due its overflow.

Initially I tried to use uint32 *shape as out parameter for js_AddScopeProperty instead of using the boolean. But that does not work. First, js_DefineNativeProperty calls the add hook for the class after js_AddScopeProperty. There is no guarantee that the hook would not cause an extra shape generation. Second, js_AddScopeProperty would also need one more parameter - pobj to find out the previous shape for the shared scope from the prototype. So the patch continues to use a boolean flag with code to find the previous shape in js_FillPropertyScope.

Comment 48

[Igor Bukanov]

(In reply to comment #37)
> Given size of first patch, I would prefer a combined patch for 1.9.0.x and
> newer branches/repos.

That is true - I have not expected initially the scope of the necessary changes to fully address the issues without hurting the performance.

Comment 49

[Igor Bukanov]

Attachment: v5 (avoiding extra locking)

here is for references the second part. I will shortly post the combined patch.

Comment 50

[Igor Bukanov]

Attachment: v5

The whole change as a single patch.

Comment 51

[Brendan Eich [:brendan]]

Comment on attachment 372877 [details] [diff] [review]
v5

>+    *vp = LOCKED_OBJ_GET_SLOT(pobj, slot);
>+    if (!SPROP_HAS_STUB_GETTER(sprop)) {
>+        /*
>+         * For API compatibility, we must set the slot with the getter's
>+         * result, so we have to make sure that it is still valid after we run
>+         * the getter.

Cite the new bug in a FIXME: comment here and possibly in js_NativeSet (different issue there, maybe needing a separate followup bug).

/be

Comment 52

[Igor Bukanov]

Attachment: v5b

adding FIXME references to the bug 488458

Comment 53

[Igor Bukanov]

landed to TM  - http://hg.mozilla.org/tracemonkey/rev/d1a4ee3d0c59

Comment 54

[Jim Blandy :jimb]

With this changeset, the time to run trace-test.js goes from 30s to 600s.

Comment 55

[Brendan Eich [:brendan]]

That's a bad performance hit -- presumably for the reason worried about in this bug of defeating the cache's predictions. A quick rebuild with

JS_PROPERTY_CACHE_METERING

defined will result at runtime in /tmp/propcache.stats. Please post it with and without this patch.

/be

Comment 56

[Jim Blandy :jimb]

Attachment: Property cache stats, pre-d1a4ee3d0c59

Comment 57

[Jim Blandy :jimb]

Attachment: Property cache stats, post-d1a4ee3d0c59

Comment 58

[Brendan Eich [:brendan]]

(In reply to comment #54)
> With this changeset, the time to run trace-test.js goes from 30s to 600s.

With -j (safe bet; without is too slow irrespective of this bug). Probably the regression is jit-only, since the propcache.stats hit rates seem uniformly better with the patch.

Igor, do you have time to look at this?

/be

Comment 59

[Brendan Eich [:brendan]]

Jim, could you get TRACEMONKEY=stats results with and without the patch for trace-test.js? Thanks,

/be

Comment 60

[Igor Bukanov]

It could be the case (In reply to comment #56)
> Created an attachment (id=372960) [details]
> Property cache stats, pre-d1a4ee3d0c59

(In reply to comment #57)
> Created an attachment (id=372961) [details]
>  Property cache stats, post-d1a4ee3d0c59

Here is the essence:

Before:

hit rates: pc 54.694% (proto 0.146141%), set 80.9384%, ini 8.99743%, full 75.65%
hit rates: pc 54.8281% (proto 0.145467%), set 80.8767%, ini 9.29241%, full 75.6896%
hit rates: pc 54.9644% (proto 0.144788%), set 80.8349%, ini 9.58439%, full 75.7307%
hit rates: pc 55.0618% (proto 0.144222%), set 80.6952%, ini 9.87342%, full 75.7491%
hit rates: pc 55.2644% (proto 0.142697%), set 80.4095%, ini 11.0652%, full 75.7904%
hit rates: pc 55.3603% (proto 0.142165%), set 80.3399%, ini 11.1934%, full 75.8119%
hit rates: pc 55.4589% (proto 0.141625%), set 80.29%, ini 11.3208%, full 75.835%
hit rates: pc 55.5232% (proto 0.141203%), set 80.1527%, ini 11.4473%, full 75.8406%
hit rates: pc 55.6686% (proto 0.14025%), set 80.0443%, ini 11.8644%, full 75.8739%
hit rates: pc 63.9982% (proto 0.113134%), set 79.8673%, ini 12.4303%, full 80.3536%


After:
hit rates: pc 89.8626% (proto 0.000519039%), set 77.8673%, ini 99.5293%, full 89.937%
hit rates: pc 94.8399% (proto 0.000264192%), set 88.9242%, ini 99.624%, full 94.8778%
hit rates: pc 96.4619% (proto 0.000177162%), set 92.6145%, ini 99.688%, full 96.4873%
hit rates: pc 97.3025% (proto 0.000133503%), set 94.4575%, ini 99.7136%, full 97.3217%
hit rates: pc 97.8127% (proto 0.000107122%), set 95.5642%, ini 99.7333%, full 97.8281%
hit rates: pc 96.4858% (proto 8.9456e-05%), set 89.6316%, ini 99.7492%, full 96.4986%
hit rates: pc 96.9828% (proto 7.68031e-05%), set 91.1106%, ini 99.7611%, full 96.9938%
hit rates: pc 97.3565% (proto 6.72911e-05%), set 92.2201%, ini 99.7708%, full 97.3661%
hit rates: pc 97.6378% (proto 5.98705e-05%), set 93.0836%, ini 99.781%, full 97.6464%
hit rates: pc 97.8726% (proto 5.3919e-05%), set 93.7747%, ini 99.7917%, full 97.8803%
hit rates: pc 98.0544% (proto 4.90407e-05%), set 94.3403%, ini 99.8024%, full 98.0615%
hit rates: pc 97.3634% (proto 4.4919e-05%), set 91.4483%, ini 99.8281%, full 97.3698%
hit rates: pc 97.4542% (proto 4.33699e-05%), set 91.5753%, ini 99.9197%, full 97.4605%
hit rates: pc 97.4644% (proto 4.26946e-05%), set 91.5808%, ini 99.9239%, full 97.4706%
hit rates: pc 97.594% (proto 3.94228e-05%), set 92.2339%, ini 99.9338%, full 97.5997%
hit rates: pc 97.0603% (proto 3.67294e-05%), set 89.945%, ini 99.9359%, full 97.0656%
hit rates: pc 97.2335% (proto 3.43803e-05%), set 90.6082%, ini 99.9378%, full 97.2385%
hit rates: pc 97.3911% (proto 3.2328e-05%), set 91.1874%, ini 99.9388%, full 97.3958%
hit rates: pc 97.538% (proto 3.05082e-05%), set 91.6992%, ini 99.9396%, full 97.5424%
hit rates: pc 97.6635% (proto 2.88836e-05%), set 92.1546%, ini 99.9403%, full 97.6677%
hit rates: pc 97.2641% (proto 2.74244e-05%), set 90.4791%, ini 99.941%, full 97.2681%
hit rates: pc 97.3956% (proto 2.61067e-05%), set 90.9494%, ini 99.9415%, full 97.3993%
hit rates: pc 97.0451% (proto 2.49087e-05%), set 89.4883%, ini 99.942%, full 97.0487%
hit rates: pc 97.1749% (proto 2.38146e-05%), set 89.9613%, ini 99.9427%, full 97.1783%
hit rates: pc 97.2888% (proto 2.2812e-05%), set 90.3938%, ini 99.9435%, full 97.2921%
hit rates: pc 97.3835% (proto 2.18718e-05%), set 90.7942%, ini 99.9462%, full 97.3866%
hit rates: pc 97.3692% (proto 2.15298e-05%), set 90.6204%, ini 99.9603%, full 97.3723%
hit rates: pc 97.3848% (proto 2.1371e-05%), set 90.6204%, ini 99.9604%, full 97.3879%
hit rates: pc 97.4048% (proto 2.12e-05%), set 90.6204%, ini 99.9603%, full 97.4079%
hit rates: pc 97.4106% (proto 2.11423e-05%), set 90.6204%, ini 99.9603%, full 97.4137%


Note that "After" has 3 times more lines meaning that GC was running more frequent. If that is indeed the case, then it would explain the jit slowdown.

Comment 61

[Mike Shaver (:shaver emeritus)]

Are we backing this out from tm?  (Did earlier versions of the patch not cause the 20x regression? Not sure how we missed that.)

Comment 62

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/5d22be2894c4

Comment 63

[Igor Bukanov]

Comment on attachment 372933 [details] [diff] [review]
v5b

>-#ifdef JS_TRACER
>-                if (!entry && TRACE_RECORDER(cx))
>-                    js_AbortRecording(cx, "SetPropUncached");
>-#endif
>+                ABORT_RECORDING(cx, "SetPropUncached");

This must be the culprit for the slow down.

Comment 64

[Igor Bukanov]

Attachment: v6

The new patch fixes that always-abort-setprop issue using ABORT_RECORDING_IF macro. It also uses ((void) 0), not ((void 0)), for non-jit case of the macros.

With the new patch I see no differences with trace-test.js timing.

Comment 65

[Brendan Eich [:brendan]]

Comment on attachment 373009 [details] [diff] [review]
v6

In the patch for bug 471214 I added ABORT_RECORDING too -- but I kept the cx param explicit. On IRC just now Igor agreed to do that and lose ABORT_RECORDING_IF, since compilers can optimize away if (pure_cond) ((void)0); without warning -- no #ifdef JS_TRACER / #endif needed.

Otherwise looks great. I can't believe I saw the loss of if (!entry) and blinked past it (distracted by checking how my ABORT_RECORDING appeared sooner than in the patch for bug 471214).

/be

Comment 66

[Igor Bukanov]

Attachment: v7

using canonical ABORT_RECORDING(cx, reason) and explicit ifs

Comment 67

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/dccd96fc69cc

Comment 68

[Andreas Gal :gal]

I think this patch might be leaking

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1239844666.1239848204.19217.gz

Comment 69

[Brendan Eich [:brendan]]

(In reply to comment #68)
> I think this patch might be leaking
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1239844666.1239848204.19217.gz

That orange cycle's blame is

http://hg.mozilla.org/tracemonkey/rev/e8c23c42db7f

or bug 488203, not this bug.

/be

Comment 70

[Andreas Gal :gal]

Backed out to see if that fixes macosx leak. Still not 100% clear the patch is guilty.

http://hg.mozilla.org/tracemonkey/rev/5bd116148175

Comment 71

[Igor Bukanov]

(In reply to comment #70)
> Backed out to see if that fixes macosx leak.

You meant Linux leak, right? On Mac the patch alone so far has not caused leaks any leaks.

Comment 72

[Igor Bukanov]

(In reply to comment #71)
> (In reply to comment #70)
> > Backed out to see if that fixes macosx leak.
> 
> You meant Linux leak, right? On Mac the patch alone so far has not caused leaks
> any leaks.

Specifically, with the patch one of the two linux tinderbox runs has reported:

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1239901924.1239908080.22165.gz

reftest
2932/0/164 LEAK

Comment 73

[Andreas Gal :gal]

Re-landed.

http://hg.mozilla.org/tracemonkey/rev/324bb9dc8372

Comment 74

[Carsten Book [:Tomcat]]

note, this patch might have caused a regression on topsites, see Bug 488734

Comment 75

[Andreas Gal :gal]

Thanks tomcat.

Comment 76

[Andreas Gal :gal]

Backed out again.

http://hg.mozilla.org/tracemonkey/rev/9c63c15b92b5

Comment 77

[Carsten Book [:Tomcat]]

(In reply to comment #76)
> Backed out again.
> 
> http://hg.mozilla.org/tracemonkey/rev/9c63c15b92b5

the backout fix Bug 488734

Comment 78

[Brendan Eich [:brendan]]

About to attach updated patch that fixes trivially the no-longer-valid assertion whose botch was sole cause of bug 488734 (AFAICT).

/be

Comment 79

[Brendan Eich [:brendan]]

Attachment: v8

Comment 80

[Igor Bukanov]

Bug 488734 happens due to the following code:

                    if (!js_SetPropertyHelper(cx, obj, id, &rval, &entry))
                        goto error;
#ifdef JS_TRACER
                    if (entry)
                        TRACE_1(SetPropMiss, entry);
#endif

With the patch this sequence is wrong as the cache will be filled before the setter call. That setter changes the shape. It triggers the assert in the recorder since the recorder checks if shape matches the cache. Note that without the patch the recorder later abort the recording due to presence of the setter.

Comment 81

[Igor Bukanov]

Comment on attachment 373393 [details] [diff] [review]
v8

The assert fix is not enough - when SetPropMis is called the entry may point to the completely unrelated sprop due to the setter activity.

The right way to fix is to replace SetPropertyHelper with function that does not call js_NativeSet at the end delegating that jot to the caller. Then the interpreter can use NATIVE_SET which would properly abort the trace before calling the setter.

Comment 82

[Brendan Eich [:brendan]]

(In reply to comment #81)
> (From update of attachment 373393 [details] [diff] [review])
> The assert fix is not enough - when SetPropMis is called the entry may point to
> the completely unrelated sprop due to the setter activity.

Yeah -- I was trying to goose the process after you dropped off irc, but it was not right.

> The right way to fix is to replace SetPropertyHelper with function that does
> not call js_NativeSet at the end delegating that jot to the caller. Then the
> interpreter can use NATIVE_SET which would properly abort the trace before
> calling the setter.

Abort under what condition? TraceRecorder::record_SetPropMiss does not abort. If you can, let's talk on IRC.

/be

Comment 83

[Brendan Eich [:brendan]]

We want to trace setter calls, the logic to abort or not will change. It should live in jstracer.cpp.

/be

Comment 84

[Igor Bukanov]

Summary of IRC discussion:

For simplicity I will update v5 (base) and fix SetPropMis problem there.

Comment 85

[Igor Bukanov]

(In reply to comment #84)
> Summary of IRC discussion:
> 
> For simplicity I will update v5 (base) and fix SetPropMis problem there.

I will finish that tomorrow. The idea is to move SetPropMis to NativeSet to call the function there before calling any setter. But that is little bit involved due to the need to unlock the object before calling the function.

Comment 86

[Igor Bukanov]

Attachment: just fixing GC hazard

I will work on the issue of stalled cache shapes in the bug 487846. The minimal fix here can be trivially back-ported to branches and can be included in 1.9.0.10. Given the complexity of js_GetSprop and the need to do new locking after running the getter the extra complexity of writing of 4 words to the memory without doing any ifs on a hot cache is nothing.

Comment 87

[Igor Bukanov]

Regarding disabling the GC for the getter or setter:

This cannot be a simple patch with few lines of code. For example, the iteration with the cycle collector has to be considered. The setter can run an arbitrary script so disabling the GC has to be propagated to the CC code as well. Another issue is __proto__ assignments done using the GC machinery. The patch has to be careful not to disable that. AFAICS all this is doable, but it requires some work. Plus it cannot be taken for granted that a port for 1.9.0 would just work.

Comment 88

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/4ea30a50bc4f

Comment 89

[Igor Bukanov]

Attachment: just fixing GC hazard for 1.9.0

Comment 90

[Igor Bukanov]

Comment on attachment 373633 [details] [diff] [review]
just fixing GC hazard for 1.9.0

The version for 1.9.0 branch required a trivial merge due to SPROP_GET->js_GetSprop changes on 1.9.1.

Comment 91

[Daniel Veditz [:dveditz]]

Given the number of iterations on the patch and 1.9.0.10 code-freeze in two days this will probably have to go in 1.9.0.11 after testing on 1.9.1

Comment 92

[Igor Bukanov]

(In reply to comment #91)
> Given the number of iterations on the patch and 1.9.0.10 code-freeze in two
> days this will probably have to go in 1.9.0.11 after testing on 1.9.1

The iterations were unrelated to the minimal fix. They were related to my initial attempt to have a fix that would address both this bug and the bug 487846. 

Moreover, the backed-out patch only had issues related to interactions with the trace recorder. There are no evidence that with the jit disabled (or no jit code as on 1.9.0) the patch would do any harm.

My worry with delaying the minimal fix until 1.9.0.11 is that the change itself is quite revealing and gives a strong hint about how to bake an exploit. On the other hand, building an exploit does require good knowledge of SpiderMonkey internals...

Comment 93

[Daniel Veditz [:dveditz]]

Comment on attachment 373633 [details] [diff] [review]
just fixing GC hazard for 1.9.0

Thanks for the explanation.

Approved for 1.9.0.10, a=dveditz for release-drivers

Comment 94

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4ea30a50bc4f

Comment 95

[Igor Bukanov]

landed to 1.9.0 branch:

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.482; previous revision: 3.481
done

Comment 96

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/faaad3e7d8cf

Comment 97

[Al Billings [:abillings - ex-MoCo]]

Bob, can you verify this fix for 1.9.0.11?

Comment 98

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-487204.js

Comment 99

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.1

Comment 100

[Daniel Veditz [:dveditz]]

we need this fix for the 1.8 branch too. Probably a simple variant of the 1.9.0 patch will work.

Comment 101

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8 version (no code change)

Comment 102

[Bob Clary [:bc] (inactive)]

v 1.9.3, 1.9.2

Comment 103

[Daniel Veditz [:dveditz]]

Comment on attachment 383224 [details] [diff] [review]
1.8 version (no code change)

Approved for 1.8.1.24, a=dveditz for release-drivers

Comment 104

[Mark Banner (:standard8)]

Checked in for 1.8.1.24:
Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.63; previous revision: 3.208.2.62

Comment 105

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.