1259730 - Can we make UnmarkGrayTracer (and similar tracers) non-recursive?

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jan de Mooij [:jandem]]

In bug 1259699 we're seeing a spike in overrecursion crashes after I adjusted the stack limit.

The crash I looked at has tons of UnmarkGrayTracer::onChild frames on the stack. That function has a stack check, but if it fails we have:

    /*
     * If we run out of stack, we take a more drastic measure: require that
     * we GC again before the next CC.
     */
    runtime()->gc.setGrayBitsInvalid();

That's not ideal because:

(1) It means our GC/CC behavior depends on the platform and compiler/PGO quirks.

(2) Bug 1259699 shows that this actually happens a lot in the wild.

(3) We end up committing a lot of stack space.

Comment 1

[Jan de Mooij [:jandem]]

Oh and this is especially bad on Windows, because MSVC PGO builds can have huge stack frames.

Comment 2

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: bug1259730-non-recursive-unmark-gray

I did some testing on unmarking a chain of JSObjects making up a singly linked list.  On an OSX debug build we fail for list length 400 and on a linux 64 optdebug build we fail for a list of length 1000.

This patch replaces the recursive gray unmarking with an implementation that uses a stack.  The stack is part of the GCRuntime and the backing allocation is only freed when we GC, so most of the time we shouldn't have to allocate.

The tests could do with some improvement but initial try results are green.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8842546 [details] [diff] [review]
bug1259730-non-recursive-unmark-gray

Review of attachment 8842546 [details] [diff] [review]:
-----------------------------------------------------------------

That came out remarkably clean. I wonder what it does for perf?

Comment 4

[Jon Coppeard (:jonco) (PTO until 3rd April)]

Attachment: bug1259730-test-unmark-gray

Add some tests for this.

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8843275 [details] [diff] [review]
bug1259730-test-unmark-gray

Review of attachment 8843275 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi-tests/testGCGrayMarking.cpp
@@ +621,5 @@
> +        Shape* shape = nobj.shape();
> +        if (!CheckCellColor(shape, color))
> +            return false;
> +
> +        // Shapes and symbols are never marekd gray.

*marked

@@ +795,5 @@
> +        // Access the 'next' property via the object's slots to avoid triggering
> +        // gray marking assertions when calling JS_GetPropertyById.
> +        NativeObject& nobj = obj->as<NativeObject>();
> +        MOZ_ASSERT(nobj.slotSpan() == 1);
> +        obj = nobj.getSlot(0).toObjectOrNull();

Heh. Nice.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68394fb5395
Make UnmarkGray use an explicit mark stack instead of recursion r=sfink

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e68394fb5395