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
Created attachment 216114 [details] [diff] [review]
Impementation

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
Created attachment 216117 [details] [diff] [review]
Impementation v2

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
Created attachment 216172 [details] [diff] [review]
Impementation v2: diff -U 10 version

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
Created attachment 216210 [details] [diff] [review]
Implementation v3

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
Created attachment 216239 [details] [diff] [review]
Patch to commit with nits addressed
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
Created attachment 216523 [details] [diff] [review]
Removal of useless v in MarkGCThingChildren
Attachment #216523 - Flags: review?(brendan)
(Assignee)

Comment 13

13 years ago
Created attachment 216544 [details] [diff] [review]
More cleanups

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.