514222 - js_GetMutableScope gives the scope a unique shape

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

js_GetMutableScope => JSScope::create => JSScope::initMinimal => js_GenerateShape

The patch in bug 471214 uncovers this ugliness in an exciting way (INITPROP immediately following NEWINIT sees a different shape each time-- and refills-- bulldozing the property cache).

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Brendan Eich [:brendan]]

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


>+        JSScope *emptyScope = OBJ_SCOPE(arrayProto)->getEmptyScope(cx, &js_SlowArrayClass);
>+        if (!emptyScope)
>+            return JS_FALSE;
>+        emptyShape = emptyScope->shape;
>+        emptyScope->drop(cx, NULL);

Digression: do we want an RAII auto-storage-class helper for this? Or are GC'ed scopes gonna happen so we can use an existing RAII rooter?

>+    JSScope *scope = JSScope::create(cx, &js_SlowArrayObjectOps, &js_SlowArrayClass, obj,
>+                                     emptyShape);

Snip 1.

>+        scope = JSScope::create(cx, ops, clasp, obj, js_GenerateShape(cx, false));

Snip 2.

>+    newscope = JSScope::create(cx, scope->map.ops, obj->getClass(), obj, scope->shape);

Snip 3.

>+JSScope::create(JSContext *cx, JSObjectOps *ops, JSClass *clasp, JSObject *obj, uint32 shape)
> {
>     JS_ASSERT(OPS_IS_NATIVE(ops));
>     JS_ASSERT(obj);
> 
>     JSScope *scope = (JSScope *) cx->malloc(sizeof(JSScope));
>     if (!scope)
>         return NULL;
> 
>     scope->map.ops = ops;
>     scope->object = obj;
>     scope->nrefs = 1;
>     scope->freeslot = JSSLOT_FREE(clasp);
>     scope->flags = cx->runtime->gcRegenShapesScopeFlag;
>     js_LeaveTraceIfGlobalObject(cx, obj);
>+    scope->shape = shape;
>     scope->initMinimal(cx);

Does passing shape down to initMinimal better express the requirement to reshape on every initMinimal?

>@@ -1554,16 +1555,17 @@ JSScope::clear(JSContext *cx)
> {
>     CHECK_ANCESTOR_LINE(this, true);
>     LIVE_SCOPE_METER(cx, cx->runtime->liveScopeProps -= entryCount);
> 
>     if (table)
>         js_free(table);
>     clearMiddleDelete();
>     js_LeaveTraceIfGlobalObject(cx, object);
>+    shape = js_GenerateShape(cx, false);

Should we not reset to the prototype's emptyScope shape?

/be

Comment 3

[Brendan Eich [:brendan]]

The "Snip N." excerpts were me trying to find common terms among the calls. It's not obvious. Both ops and clasp seem necessary and not deducible from obj, but could we get ops from clasp->getObjectOps ? clasp->getObjectOps(cx, clasp) : &js_ObjectOps and save a param (and common some code)?

/be

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: v2

Deferred RAII (until scopes are GC'd) and eliminating the ops parameter, took the other proposed changes.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 398231 [details] [diff] [review]
v2

>@@ -1554,17 +1554,27 @@ JSScope::clear(JSContext *cx)
> {
>     CHECK_ANCESTOR_LINE(this, true);
>     LIVE_SCOPE_METER(cx, cx->runtime->liveScopeProps -= entryCount);
> 
>     if (table)
>         js_free(table);
>     clearMiddleDelete();
>     js_LeaveTraceIfGlobalObject(cx, object);
>-    initMinimal(cx);
>+
>+    JSClass *clasp = object->getClass();
>+    JSObject *proto = object->getProto();
>+    uint32 newShape;
>+    if (!(proto &&
>+          clasp == proto->getClass() &&
>+          OBJ_SCOPE(proto)->getEmptyScopeShape(cx, clasp, &newShape))) {
>+        newShape = js_GenerateShape(cx, false);
>+    }

This is odd-looking and suppresses an error that we reasoned "can't happen" (for some IRC value of "reasoned" -- more permanent proofoid wanted). How about

    if (proto && proto->getClass() == clasp) {
#ifdef DEBUG
        bool ok =
#endif
        OBJ_SCOPE(proto)->getEmptyScopeShape(cx, clasp, &newShape);
        JS_ASSERT(ok);
    } else {
        newShape = js_GenerateShape(cx, false);
    }

instead? r=me with that. Thanks,

/be

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/55567293369c

Comment 7

[Julian Seward [:jseward]]

Comparing 32124:55567293369c with predecessor 32123:c78bb369e198
suggests this patch does not introduce any new leaks.

vTRUNK --tool=memcheck --smc-check=all ./D32123/js/src/BuildR/js 
   -j -e 'gSkipSlowTests=true' ./trace-test.js

in both cases gives
     in use at exit: 163,915 bytes in 2,282 blocks
   total heap usage: 176,008 allocs, 173,726 frees, 141,309,692 bytes allocated

 LEAK SUMMARY:
    definitely lost: 150,751 bytes in 2,233 blocks
    indirectly lost: 13,164 bytes in 49 blocks
      possibly lost: 0 bytes in 0 blocks
    still reachable: 0 bytes in 0 blocks
         suppressed: 0 bytes in 0 blocks

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/55567293369c

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c82140f985a6