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

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jandem, Assigned: jonco)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

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.
Oh and this is especially bad on Windows, because MSVC PGO builds can have huge stack frames.
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.
Assignee: nobody → jcoppeard
Attachment #8842546 - Flags: review?(sphink)
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?
Attachment #8842546 - Flags: review?(sphink) → review+
Add some tests for this.
Attachment #8843275 - Flags: review?(sphink)
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.
Attachment #8843275 - Flags: review?(sphink) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68394fb5395
Make UnmarkGray use an explicit mark stack instead of recursion r=sfink
https://hg.mozilla.org/mozilla-central/rev/e68394fb5395
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.