We don't cache access to shared properties in the property cache

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

({fixed1.9.1})

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 4 obsolete attachments)

This makes access to XPConnect getters/setters slow, and inhibits tracing.
Igor also pointed out that we should fill the prop cache before calling getters/setters, since they can have arbitrary effects (including gc, which invalidates the shape info stored in the prop cache).
Posted patch patch (fails mandelbrot) (obsolete) — Splinter Review
(In reply to comment #2)
> Created an attachment (id=375087) [details]
> patch (fails mandelbrot)

The patch calls SetPropHit after calling NativeSet or its inlined forms in js_Interpret. This is not good since the setter can override the cached entry. I suspect that the right thing to do is to call SetPropHit before any setter activity.
I was worried that obj->scope isn't up to date yet in all cases if we call before the set.
This is still failing mandelbrot (half way through it). Maybe GC related?
Attachment #375087 - Attachment is obsolete: true
Attachment #375091 - Attachment is obsolete: true
I think it is just a bad idea to call SetPropHit with a locked object. AFAICS fixing that is straightforward if NATIVE_SET would be changed to unlock the object and call SetPropHit.
I was thinking about this but I can't find a reason why we shouldn't call SetPropHit with a locked object. Why do you think should we unlock?
Attachment #375092 - Flags: review?(igor)
Attachment #375092 - Flags: review?(igor) → review+
Comment on attachment 375092 [details] [diff] [review]
added missing setprophit to definenativeproperty

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -4592,19 +4592,19 @@ js_Interpret(JSContext *cx)
>                                  * 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 ||
>                                     SCOPE_HAS_PROPERTY(scope, sprop)) {
>                                     PCMETER(cache->pchits++);
>                                     PCMETER(cache->setpchits++);
>+                                    TRACE_2(SetPropHit, entry, sprop);
>                                     NATIVE_SET(cx, obj, sprop, &rval);

Nit: move TRACE_2(SetPropHit, entry, sprop) into NATIVE_SET itself.

>                 if (!atom)
>                     LOAD_ATOM(0);
>                 id = ATOM_TO_JSID(atom);
>                 if (entry) {
>                     entry = js_SetPropertyHelper(cx, obj, id, true, &rval);

...
> 
>                 entry = JS_UNLIKELY(atom == cx->runtime->atomState.protoAtom)
>                         ? js_SetPropertyHelper(cx, obj, id, true, &rval)
>                         : js_DefineNativeProperty(cx, obj, id, rval, NULL, NULL,
>                                                   JSPROP_ENUMERATE, 0, 0, NULL,
>                                                   true);
>                 if (!entry)
>                     goto error;

Nit: with the patch there is no point to return the proeprty cache entry from js_SetPropertyHelper as it is used only as an error flag. So change the return type of js_SetPropertyHelper (and similarly js_DefineNativeProperty) back to JSBool. 

>@@ -4528,16 +4529,18 @@ js_SetPropertyHelper(JSContext *cx, JSOb

Nit: comment that all non-error return paths in js_SetPropertyHelper must go via SetPropHit. Alternatively it would be nice to reorganize the code so that happens automatically via some goto or break. 

r+ with the nits addressed and assuming that a merge with recent changes to js_FillPropertyCache signature would be trivial. If not, I will do promptly review om the updated patch.
I'd like to see a final patch too. Thanks,

/be
Attachment #375092 - Attachment is obsolete: true
Attachment #375146 - Flags: review?(igor)
Comment on attachment 375146 [details] [diff] [review]
re-based for tip and addressed nits

This definitively needs a new round of reviews. Changes were more invasive than expected.
Attachment #375146 - Flags: review?(brendan)
Comment on attachment 375146 [details] [diff] [review]
re-based for tip and addressed nits

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -3565,16 +3565,17 @@ js_Interpret(JSContext *cx)
> #define NATIVE_SET(cx,obj,sprop,vp)                                           \
>     JS_BEGIN_MACRO                                                            \
>+        TRACE_2(SetPropHit, entry, sprop);                                    \

Nit: pass entry as a parameter here.

>-JSPropCacheEntry *
>+/*
>+ * Note: all non-error exits in this function must notify the tracer using
>+ * SetPropHit.
>+ */

Comment nit: the tracer must be notified only when cacheResult is true.

>@@ -4544,33 +4548,35 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>             /*
>-             * 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.
>+             * Don't clone a shared prototype property.
>              */

Nit: change the comment style to /* one line */

>+                if (!js_SetSprop(cx, sprop, obj, vp))
>+                    return JS_FALSE;
>+
>+                return JS_TRUE;

Nit: use just return js_SetSprop()
Attachment #375146 - Flags: review?(igor) → review+
Posted patch patchSplinter Review
igor's nits. Passed try-server. Waiting for brendan's review.
Attachment #375146 - Attachment is obsolete: true
Attachment #375207 - Flags: review?(brendan)
Attachment #375146 - Flags: review?(brendan)
Comment on attachment 375207 [details] [diff] [review]
patch

>+                if (!(JS_UNLIKELY(atom == cx->runtime->atomState.protoAtom)
>+                      ? js_SetPropertyHelper(cx, obj, id, true, &rval)
>+                      : js_DefineNativeProperty(cx, obj, id, rval, NULL, NULL,
>+                                                JSPROP_ENUMERATE, 0, 0, NULL,
>+                                                true)))
>+                    goto error;

Brace consequent of if with multiline condition, just as for any multi-line then or else part.

>@@ -3806,33 +3805,31 @@ js_DefineNativeProperty(JSContext *cx, J
>     if (SPROP_HAS_VALID_SLOT(sprop, scope))
>         LOCKED_OBJ_WRITE_BARRIER(cx, obj, sprop->slot, value);
> 
>     /* XXXbe called with lock held */
>     ADD_PROPERTY_HELPER(cx, clasp, obj, scope, sprop, &value,
>                         js_RemoveScopeProperty(cx, scope, id);
>                         goto bad);
> 
>-    entry = JS_NO_PROP_CACHE_FILL;
>     if (cacheResult) {
>         JS_ASSERT_NOT_ON_TRACE(cx);
>-        if (!(attrs & JSPROP_SHARED))
>-            entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);
>-        else
>-            PCMETER(JS_PROPERTY_CACHE(cx).nofills++);

Is nofills still used with this patch applied? If not, remove.

Note how filling for a shared sprop is gonna use (0,0) proto/scope-height coords. But this is likely an sprop on a constructor's prototype and we will want to hit it from instances of the constructor that delegate to the prototype. But those property cache tests will occur from different (pc,shape) keys than whatever the pc might be for this call.

That was one reason why I did not fill shared sprops here, and it may still apply. No point in filling what won't be hit, colliding with other likely-good cache lines.

>+        JSPropCacheEntry *entry;
>+        entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);

Combine declaration and initialization.

>@@ -4524,17 +4526,19 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>              * must likewise re-task flags further below for the other 'goto
>              * read_only_error;' case.
>              */
>             flags = JSREPORT_ERROR;
>             if (attrs & JSPROP_READONLY) {
>                 if (!JS_HAS_STRICT_OPTION(cx)) {
>                     /* Just return true per ECMA if not in strict mode. */
>                     PCMETER(cacheResult && JS_PROPERTY_CACHE(cx).rofills++);
>-                    return JS_NO_PROP_CACHE_FILL;
>+                    if (cacheResult)
>+                        TRACE_2(SetPropHit, JS_NO_PROP_CACHE_FILL, sprop);

Wouldn't a direct ABORT_RECORDING call be better, or do you consolidate the reason string this way?

>@@ -4543,34 +4547,31 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>              * We found id in a prototype object: prepare to share or shadow.
>              *
>              * NB: Thanks to the immutable, garbage-collected property tree
>              * maintained by jsscope.c in cx->runtime, we needn't worry about
>              * sprop going away behind our back after we've unlocked scope.
>              */
>             JS_UNLOCK_SCOPE(cx, scope);
> 
>+            /* Don't clone a shared prototype property. */
>             if (attrs & JSPROP_SHARED) {
>+                if (cacheResult) {
>+                    JS_ASSERT_NOT_ON_TRACE(cx);
>+                    JSPropCacheEntry *entry;
>+                    entry = js_FillPropertyCache(cx, obj, 0, protoIndex, pobj, sprop, false);

Combine decl and init -- wrap trailing args if overlong by local code conventions.

Here is where caching shared properties can pay off, big-time.

>     if (cacheResult) {
>         JS_ASSERT_NOT_ON_TRACE(cx);
>-        if (!(attrs & JSPROP_SHARED))
>-            entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);
>-        else
>-            PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
>-    }
>+        JSPropCacheEntry *entry;
>+        entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);

Decl+init.

> TraceRecorder::record_SetPropHit(JSPropCacheEntry* entry, JSScopeProperty* sprop)

Comment that this is called with the object at sp[-2] locked.

/be
shared sprops don't always use (0,0). If you hit one level up, thats how we enter it. What do you want me to do here?

> Note how filling for a shared sprop is gonna use (0,0) proto/scope-height
> coords. But this is likely an sprop on a constructor's prototype and we will
> want to hit it from instances of the constructor that delegate to the
> prototype. But those property cache tests will occur from different (pc,shape)
> keys than whatever the pc might be for this call.
> 
> That was one reason why I did not fill shared sprops here, and it may still
> apply. No point in filling what won't be hit, colliding with other likely-good
> cache lines.
>
(In reply to comment #15)
> That was one reason why I did not fill shared sprops here, and it may still
> apply.

The code in the question is from js_DerfineNativeProperty. It is called from js_Interpret only from the INITPROP bytecode. In this case (attrs & JSPROP_SHARED) is false. Thus the current code without the patch,

    if (cacheResult) {
        JS_ASSERT_NOT_ON_TRACE(cx);
        if (!(attrs & JSPROP_SHARED))
            entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);
        else
            PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
    }

is doing useless (attrs & JSPROP_SHARED) check since cacheResult implies attrs == JSPROP_ENUMERATE.

So the patch is doing the right thing and removes useless check. 

 No point in filling what won't be hit, colliding with other likely-good
> cache lines.
> 
> >+        JSPropCacheEntry *entry;
> >+        entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);
> 
> Combine declaration and initialization.
> 
> >@@ -4524,17 +4526,19 @@ js_SetPropertyHelper(JSContext *cx, JSOb
> >              * must likewise re-task flags further below for the other 'goto
> >              * read_only_error;' case.
> >              */
> >             flags = JSREPORT_ERROR;
> >             if (attrs & JSPROP_READONLY) {
> >                 if (!JS_HAS_STRICT_OPTION(cx)) {
> >                     /* Just return true per ECMA if not in strict mode. */
> >                     PCMETER(cacheResult && JS_PROPERTY_CACHE(cx).rofills++);
> >-                    return JS_NO_PROP_CACHE_FILL;
> >+                    if (cacheResult)
> >+                        TRACE_2(SetPropHit, JS_NO_PROP_CACHE_FILL, sprop);
> 
> Wouldn't a direct ABORT_RECORDING call be better, or do you consolidate the
> reason string this way?
> 
> >@@ -4543,34 +4547,31 @@ js_SetPropertyHelper(JSContext *cx, JSOb
> >              * We found id in a prototype object: prepare to share or shadow.
> >              *
> >              * NB: Thanks to the immutable, garbage-collected property tree
> >              * maintained by jsscope.c in cx->runtime, we needn't worry about
> >              * sprop going away behind our back after we've unlocked scope.
> >              */
> >             JS_UNLOCK_SCOPE(cx, scope);
> > 
> >+            /* Don't clone a shared prototype property. */
> >             if (attrs & JSPROP_SHARED) {
> >+                if (cacheResult) {
> >+                    JS_ASSERT_NOT_ON_TRACE(cx);
> >+                    JSPropCacheEntry *entry;
> >+                    entry = js_FillPropertyCache(cx, obj, 0, protoIndex, pobj, sprop, false);
> 
> Combine decl and init -- wrap trailing args if overlong by local code
> conventions.
> 
> Here is where caching shared properties can pay off, big-time.
> 
> >     if (cacheResult) {
> >         JS_ASSERT_NOT_ON_TRACE(cx);
> >-        if (!(attrs & JSPROP_SHARED))
> >-            entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);
> >-        else
> >-            PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
> >-    }
> >+        JSPropCacheEntry *entry;
> >+        entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);
> 
> Decl+init.
> 
> > TraceRecorder::record_SetPropHit(JSPropCacheEntry* entry, JSScopeProperty* sprop)
> 
> Comment that this is called with the object at sp[-2] locked.
> 
> /be
[I accidentally hit the submit button on the prev comment, here is the proper version]

(In reply to comment #15)
> That was one reason why I did not fill shared sprops here, and it may still
> apply.

The code in the question is from js_DerfineNativeProperty. It is called from
js_Interpret only from the INITPROP bytecode. In this case (attrs &
JSPROP_SHARED) is false. Thus the current code without the patch,

    if (cacheResult) {
        JS_ASSERT_NOT_ON_TRACE(cx);
        if (!(attrs & JSPROP_SHARED))
            entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added);
        else
            PCMETER(JS_PROPERTY_CACHE(cx).nofills++);
    }

is doing useless (attrs & JSPROP_SHARED) check since cacheResult implies attrs
== JSPROP_ENUMERATE.

So the patch is doing the right thing and removes the check that is always true.
Commenting only to clarify misunderstandings, real or potential:

> If you hit one level up, thats how we enter it.

You won't hit (0,1) or other non-(0,0) from the pc pointing at JSOP_INITPROP. But I missed the cacheResult logic strictly inducing !(attrs & JSPROP_SHARED).

> What do you want me to do here?

Nothing, Igor points out that the cacheResult flag is enough.

/be
Attachment #375207 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/3519ddacbe2e
Whiteboard: fixed-in-tracemonkey
Blocks: 480192
(In reply to comment #19)
> You won't hit (0,1) or other non-(0,0) from the pc pointing at JSOP_INITPROP.
But I missed the cacheResult logic strictly inducing !(attrs & JSPROP_SHARED).

Note that INITPROP for JSOP_(GETTER|SETTER) does not use caching and calls OBJ_DEFINE_PROPERTY implying !cacheResult in js_DefineNativeProperty. But even if getter/setter initializers would be cached, then the same argument about caching predictive shape mutations for ordinary property initialization should apply AFAICS when defining getters or setters.
No longer blocks: 480192
Blocks: 488458
(In reply to comment #21)
> (In reply to comment #19)
> > You won't hit (0,1) or other non-(0,0) from the pc pointing at JSOP_INITPROP.
> But I missed the cacheResult logic strictly inducing !(attrs & JSPROP_SHARED).
> 
> Note that INITPROP for JSOP_(GETTER|SETTER) does not use caching and calls
> OBJ_DEFINE_PROPERTY implying !cacheResult in js_DefineNativeProperty. But even
> if getter/setter initializers would be cached, then the same argument about
> caching predictive shape mutations for ordinary property initialization should
> apply AFAICS when defining getters or setters.

No question -- the only issue (bogus, it turned out) could be premature caching of properties defined from an uncorrelated pc.

/be
http://hg.mozilla.org/mozilla-central/rev/3519ddacbe2e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 491013
Depends on: 491040
Depends on: 558541
You need to log in before you can comment on or make changes to this bug.