[Webkit-unassigned] [Bug 70995] The GC should be parallel

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70995


Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #112990|review?                     |review+
               Flag|                            |




--- Comment #28 from Geoffrey Garen <ggaren at apple.com>  2011-10-31 17:53:27 PST ---
(From update of attachment 112990)
View in context: https://bugs.webkit.org/attachment.cgi?id=112990&action=review

r=me, with some comments below.

Windows build issue:


3>JavaScriptCore.exp : error LNK2001: unresolved external symbol "public: void __thiscall JSC::MarkStackArray::expand(void)" (?expand at MarkStackArray@JSC@@QAEXXZ)

> Source/JavaScriptCore/heap/Heap.cpp:618
> +            visitor.donateAndDrain();

Not for this patch, but it's really becoming apparent to me that draining should be automatic in the visitor: Once you append N items, the visitor should drain. The visitor has the best information about how full it is at any given time. Plus, this change would seriously declutter-ify calling code.

> Source/JavaScriptCore/heap/MarkStack.cpp:348
> +            for (unsigned countdown = Heuristics::minimumNumberOfScansBetweenRebalance; m_stack.canRemoveLast() && countdown-- > 0;)

You can elide "> 0" here.

> Source/JavaScriptCore/heap/MarkStack.cpp:370
> +    if (Heuristics::numberOfGCMarkers > 1) {

You can change this to an early return, to reduce the indentation level of the rest of the code. WebKit coding style prefers early return where possible for that reason.

> Source/JavaScriptCore/heap/MarkStack.cpp:399
> +                    // If we reached termination, then return.
> +                    if (!m_shared.m_numberOfActiveParallelMarkers && m_shared.m_sharedMarkStack.isEmpty())
> +                        return;

You can just change the condition above to "return" instead of "break", and remove this code.

> Source/JavaScriptCore/heap/MarkStack.h:166
> +    class MarkStackSharedData {

I think you should call out the threadiness of this code: "MarkStackThreadSharedData".

> Source/JavaScriptCore/heap/MarkStack.h:260
> +            if ((unsigned)m_opaqueRoots.size() < Heuristics::opaqueRootMergeThreshold)

C++-style static_cast, please.

Slightly better to upcast opaqueRootMergeThreshold, rather than down casting m_opaqueRoots.size(), to avoid loss of information.

> Source/JavaScriptCore/heap/MarkStack.h:311
> +    inline void MarkStack::addOpaqueRoot(void* root)
>      {
> -        return m_opaqueRoots.add(root).second;
> +#if ENABLE(PARALLEL_GC)
> +        mergeOpaqueRootsIfProfitable();
> +#endif
> +        m_opaqueRoots.add(root);
>      }

I'm surprised that's profitable to merge opaque roots eagerly. Nothing can be done with the set of opaque roots until the end of parallel work, anyway. Could be simpler just to omit eager merging.

Do you know why this is profitable?

> Source/JavaScriptCore/runtime/Heuristics.cpp:32
> +#if PLATFORM(MAC) && ENABLE(PARALLEL_GC)

OS(UNIX) or OS(DARWIN) is better than PLATFORM(MAC) here.

> Source/JavaScriptCore/runtime/Heuristics.cpp:177
> +#if PLATFORM(MAC) && ENABLE(PARALLEL_GC)
> +    int name[2];
> +    size_t valueSize = sizeof(cpusToUse);
> +    name[0] = CTL_HW;
> +    name[1] = HW_AVAILCPU;
> +    sysctl(name, 2, &cpusToUse, &valueSize, 0, 0);

OS(UNIX) or OS(DARWIN) is better than PLATFORM(MAC) here.

> Source/JavaScriptCore/wtf/Bitmap.h:40
> +#if PLATFORM(MAC)

CPU(X86) is better than PLATFORM(MAC) here.

> Source/JavaScriptCore/wtf/Platform.h:1078
> +#if !ENABLE(PARALLEL_GC) && PLATFORM(MAC)
> +#define ENABLE_PARALLEL_GC 1

This should be "#!defined(ENABLE_PARALLEL_GC)" instead of "!ENABLE(PARALLEL_GC)" (so you can manually define to disable parallel GC).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.