[webkit-reviews] review granted: [Bug 70995] The GC should be parallel : [Attachment 112990] the patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 70995: The GC should be parallel
https://bugs.webkit.org/show_bug.cgi?id=70995

Attachment 112990: the patch
https://bugs.webkit.org/attachment.cgi?id=112990&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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).