This is a followup for the bug 324278 to tune the code.

The patch does the following: 1. UnmarkedGCThingFlags is replaced by direct calls to js_GetGCThingFlags leading to code with less checks. 2. The new version of js_MarkGCThing avoids an extra AddThingToUnscannedBag call when called from the mark-phase callback. 3. Intermediate macro GC_CAN_RECURSE is eliminated.

The previous patch was incomplete: I should learn not to forget to use "quilt refresh" before attaching the patches.

Comment on attachment 216117 [details] [diff] [review] Impementation v2 Oops, where did the use of js_LiveThingToFind go? /be

In reply to comment #3) > (From update of attachment 216117 [details] [diff] [review] [edit]) > Oops, where did the use of js_LiveThingToFind go? I moved it to js_MarkNamedGCThing as this function is called to mark all GC things when GC_MARK_DEBUG is defined. But that was not commented sufficiently. This is easy to fix but first I would like to clarify the following: GC dumps (and the patch does not change it) js_LiveThingToFind each time it is reached through the graph and not only once when the thing is marked to get a better picture of all references to the thing. Right?

(In reply to comment #4) > This is easy to fix but first I would like to clarify the following: > > GC dumps (and the patch does not change it) js_LiveThingToFind each time it is > reached through the graph and not only once when the thing is marked to get a > better picture of all references to the thing. Right? Right -- reviewing in a minute. /be

Comment on attachment 216172 [details] [diff] [review] Impementation v2: diff -U 10 version Sorry, dunno how I missed it in the last patch -- bad find-as-you-type wraparound? >@@ -1065,24 +1065,27 @@ > js_MarkNamedGCThing(JSContext *cx, void *thing, const char *name) > { > GCMarkNode markNode; > > markNode.thing = thing; > markNode.name = name; > markNode.next = NULL; > markNode.prev = (GCMarkNode *)cx->gcCurrentMarkNode; > if (markNode.prev) > markNode.prev->next = &markNode; > cx->gcCurrentMarkNode = &markNode; > >+ if (thing && js_LiveThingToFind == thing) >+ gc_dump_thing(cx, thing, stderr); >+ > js_MarkGCThing(cx, thing); > > if (markNode.prev) > markNode.prev->next = NULL; > cx->gcCurrentMarkNode = markNode.prev; > } If thing is null, why not return early, ASAP, and save all that work? r=me with that, thanks. /be

Better comments and early return from js_MarkNamedGCThing

Comment on attachment 216210 [details] [diff] [review] Implementation v3 >+ if (js_LiveThingToFind == thing) { Nit: prevailing style has slight preference for local and more rapidly varying arg (thing) on left of ==, invariant (js_LiveThingToFind) on the right. > void > js_MarkGCThing(JSContext *cx, void *thing) > { > uint8 *flagp; >- JSBool fromCallback; > >- flagp = UnmarkedGCThingFlags(GC_MARK_DEBUG_CX(cx, thing)); >- if (!flagp) >+ if (thing == NULL) Prevailing style tests !thing, not thing == NULL. Fix these nits and r=me. Thanks again, good to see simplification. /be

I committed the patch from attachment 216239 [details] [diff] [review].

This is another leftover from bug 324278. After the changes the local variable v in MarkGCThingChildren is no longer effectively used and should be removed.

This patch eliminates NextUnmarkedGCThing and simplifies the slot scanning loop in MarkGCThingChildren down to the fragment bellow. There the code to extract the slot name is moved to a separated GC_MARK_DEBUG GetObjSlotName function. thing = NULL; flagp = NULL; #ifdef GC_MARK_DEBUG scope = OBJ_IS_NATIVE(obj) ? OBJ_SCOPE(obj) : NULL; #endif for (; vp != end; ++vp) { v = *vp; if (!JSVAL_IS_GCTHING(v) || v == JSVAL_NULL) continue; next_thing = JSVAL_TO_GCTHING(v); if (next_thing == thing) continue; next_flagp = js_GetGCThingFlags(next_thing); if (*next_flagp & GCF_MARK) continue; JS_ASSERT(*next_flagp != GCF_FINAL); if (thing) { #ifdef GC_MARK_DEBUG GC_MARK(cx, thing, name); #else *flagp |= GCF_MARK; MarkGCThingChildren(cx, thing, flagp, JS_TRUE); #endif if (*next_flagp & GCF_MARK) { /* * This happens when recursive MarkGCThingChildren marks * the thing with flags referred by *next_flagp. */ thing = NULL; continue; } } #ifdef GC_MARK_DEBUG GetObjSlotName(scope, vp - obj->slots, name, sizeof name); #endif thing = next_thing; flagp = next_flagp; } if (thing) { /* * thing came from the last unmarked GC-thing slot and we * can optimize tail recursion. * * Since we already know that there is enough C stack space, * we clear shouldCheckRecursion to avoid extra checking in * RECURSION_TOO_DEEP. */ shouldCheckRecursion = JS_FALSE; goto on_tail_recursion; }

Brendan, do you have time to look for this? I could reassign this to somebody else according to "cvs log jsgc.c".

(In reply to own comment #14) > Brendan, do you have time to look for this? Here "this" means "review for the last patch" ;)

Comment on attachment 216544 [details] [diff] [review] More cleanups r=me, sorry for the delay! (Traveling.) /be

I committed the patch from attachment 216544 [details] [diff] [review]