Followup patch for GC without recursion

RESOLVED FIXED

Status

()

--
enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
x86
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

13 years ago
This is a followup for the bug 324278 to tune the code.
(Assignee)

Comment 1

13 years ago
Posted patch Impementation (obsolete) — Splinter Review
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.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #216114 - Flags: review?(brendan)
(Assignee)

Comment 2

13 years ago
Posted patch Impementation v2 (obsolete) — Splinter Review
The previous patch was incomplete: I should learn not to forget to use "quilt refresh" before attaching the patches.
Attachment #216114 - Attachment is obsolete: true
Attachment #216117 - Flags: review?(brendan)
Attachment #216114 - Flags: review?(brendan)
Comment on attachment 216117 [details] [diff] [review]
Impementation v2

Oops, where did the use of js_LiveThingToFind go?

/be
Attachment #216117 - Flags: review?(brendan) → review-
(Assignee)

Comment 4

13 years ago
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?
Attachment #216117 - Attachment is obsolete: true
(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
Attachment #216172 - Flags: review+
(Assignee)

Comment 7

13 years ago
Posted patch Implementation v3 (obsolete) — Splinter Review
Better comments and early return from js_MarkNamedGCThing
Attachment #216172 - Attachment is obsolete: true
Attachment #216210 - Flags: review?(brendan)
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
Attachment #216210 - Flags: review?(brendan) → review+
(Assignee)

Comment 9

13 years ago
Attachment #216210 - Attachment is obsolete: true
(Assignee)

Comment 10

13 years ago
I committed the patch from attachment 216239 [details] [diff] [review].
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

13 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

13 years ago
Attachment #216523 - Flags: review?(brendan)
(Assignee)

Comment 13

13 years ago
Posted patch More cleanupsSplinter Review
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;
        }
Attachment #216523 - Attachment is obsolete: true
Attachment #216544 - Flags: review?(brendan)
Attachment #216523 - Flags: review?(brendan)

Updated

13 years ago
Summary: Addon for GC without recursion → Followup patch for GC without recursion
(Assignee)

Comment 14

13 years ago
Brendan, do you have time to look for this? I could reassign this to somebody else according to "cvs log jsgc.c".
Status: REOPENED → ASSIGNED
(Assignee)

Comment 15

13 years ago
(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
Attachment #216544 - Flags: review?(brendan) → review+
(Assignee)

Comment 17

13 years ago
I committed the patch from attachment 216544 [details] [diff] [review]
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.