Closed
Bug 514222
Opened 15 years ago
Closed 15 years ago
js_GetMutableScope gives the scope a unique shape
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 1•15 years ago
|
||
Assignee: general → jorendorff
Attachment #398191 -
Flags: review?(brendan)
Comment 2•15 years ago
|
||
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•15 years ago
|
||
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
Assignee | ||
Comment 4•15 years ago
|
||
Deferred RAII (until scopes are GC'd) and eliminating the ops parameter, took the other proposed changes.
Attachment #398191 -
Attachment is obsolete: true
Attachment #398231 -
Flags: review?(brendan)
Attachment #398191 -
Flags: review?(brendan)
Comment 5•15 years ago
|
||
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
Attachment #398231 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 6•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/55567293369c
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Updated•15 years ago
|
Comment 7•15 years ago
|
||
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•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/55567293369c
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 9•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c82140f985a6
status1.9.2:
--- → beta1-fixed
Flags: wanted1.9.2+
You need to log in
before you can comment on or make changes to this bug.
Description
•