[webkit-reviews] review granted: [Bug 179934] GC constraint solving should be parallel : [Attachment 328317] the patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 179934: GC constraint solving should be parallel
https://bugs.webkit.org/show_bug.cgi?id=179934

Attachment 328317: the patch

https://bugs.webkit.org/attachment.cgi?id=328317&action=review




--- Comment #56 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 328317
  --> https://bugs.webkit.org/attachment.cgi?id=328317
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328317&action=review

I know the code usually doesn't, but it would be nice to use `final` where you
can.

I haven't really wrapped my mind around how the GC races are correct, so modulo
that the code looks good module the union UB comments. r=me with that fixed.

> Source/JavaScriptCore/heap/ConstraintConcurrency.h:41
> +inline void printInternal(PrintStream& out, JSC::ConstraintConcurrency
concurrency)

I think this can be in the JSC namespace above, and be found through ADL?

> Source/JavaScriptCore/heap/ConstraintParallelism.h:41
> +inline void printInternal(PrintStream& out, JSC::ConstraintParallelism
parallelism)

Ditto.

> Source/JavaScriptCore/heap/Heap.cpp:618
> +	       dataLog("FATAL: Visitor ", RawPointer(&visitor), " is not
empty!\n");

I don't know what to do if this fires. Could you make it more helpful? Also
doesn't seem fatal as the message implies, since it doesn't actually kill
anything.

> Source/JavaScriptCore/heap/Heap.cpp:2599
> +	   [this, lastVersion = static_cast<uint64_t>(0)] (SlotVisitor&
slotVisitor) mutable {

Neat. I never use mutable lambdas, I totally should.

> Source/JavaScriptCore/heap/Heap.cpp:2613
> +	       lastVersion = m_phaseVersion;

m_phaseVersion can be modified concurrently to this executing, right? Is it
problematic if this read of m_phaseVersion is the same / different from the one
at the top of the function? Seems like not but I want to make sure you don't
need something like READ_ONCE.

> Source/JavaScriptCore/heap/Heap.cpp:2865
> +    }

I don't understand why you need to lock after setting and running the bonus
task, and why you're waiting for the refcount this way. Could you add a
comment?

> Source/JavaScriptCore/heap/HeapInlines.h:269
> +	   func(*slotVisitor);

Is there some Science or Heuristic behind the order of the func() calls?

> Source/JavaScriptCore/heap/MarkStackMergingConstraint.h:35
> +    ~MarkStackMergingConstraint();

override

> Source/JavaScriptCore/heap/MarkingConstraint.cpp:34
> +static constexpr bool verbose = false;

With unified builds this needs to be localized, no?

> Source/JavaScriptCore/heap/MarkingConstraint.cpp:71
> +    return 0;

Why is that correct?

> Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:126
> +    if (order[index]->quickWorkEstimate(m_mainVisitor)) {

I find testing floating-point like this weird. Seems like you want
quickWorkEstimate > 0. here.

> Source/JavaScriptCore/heap/SimpleMarkingConstraint.h:44
> +    JS_EXPORT_PRIVATE ~SimpleMarkingConstraint();

override

> Source/JavaScriptCore/heap/SlotVisitor.h:97
> +    bool addOpaqueRoot(void*); // Returns true if the root was new.

Want to use a named enum class instead of a comment?

> Source/WTF/wtf/Atomics.h:358
> +    return opaqueMixture(arguments...) + u.copy;

Your usage of union is UB. You want to operate through bitwise_cast or memcpy /
memset.

> Source/WTF/wtf/Atomics.h:371
> +    // phantom depenendency.

Typo "dependency".

> Source/WTF/wtf/ConcurrentPtrHashSet.cpp:82
> +		   return add(ptr);

You can exhaust the stack here if you're having a real bad time?

> Source/WTF/wtf/ConcurrentPtrHashSet.h:99
> +	   Atomic<void*> array[1];

We can't use the C extension `Atomic<void*> array[];` ?

> Source/WTF/wtf/ConcurrentPtrHashSet.h:119
> +	   return u.ptr;

As above, you want memset here, then u.value = value.

> Source/WTF/wtf/CountingLock.h:55
> +// The latter is important for us because some GC paths are known to be
sensitive to fences on ARM.

This is neat. As we discussed on IRC in some cases seqlock might be more
efficient. I have a  version here:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0690r0.html#seqlock