[webkit-reviews] review granted: [Bug 217274] Implement a GC verifier. : [Attachment 420900] proposed patch.

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 217274: Implement a GC verifier.
https://bugs.webkit.org/show_bug.cgi?id=217274

Attachment 420900: proposed patch.

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




--- Comment #37 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 420900
  --> https://bugs.webkit.org/attachment.cgi?id=420900
proposed patch.

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

I think I would have used virtual dispatch a lot more. But if this works, then
let’s just do it.

> Source/JavaScriptCore/dfg/DFGPlan.cpp:716
> +bool Plan::isKnownToBeLiveDuringGC(Visitor& visitor)

It’s surprising that stuff like this needs the templatization. Seems like you
would have gotten away with virtual calls in paths like this.

> Source/JavaScriptCore/dfg/DFGSafepoint.cpp:105
> +bool Safepoint::isKnownToBeLiveDuringGC(Visitor& visitor)

And stuff like this.

> Source/JavaScriptCore/heap/Heap.cpp:2785
> +	   MAKE_MARKING_CONSTRAINT_EXECUTOR_PAIR(([this] (auto& visitor) {

surprising that you needed templatization for stuff like this. Did you
benchmark it?

> Source/JavaScriptCore/heap/VerifierSlotVisitor.cpp:341
> +	   cell->methodTable(vm())->visitChildren(const_cast<JSCell*>(cell),
*this);

You could just call this directly without the switch. The switch is a silly
perf optimization.

> Source/JavaScriptCore/interpreter/ShadowChicken.h:199
> +    void visitChildren(AbstractSlotVisitor&);

Why can’t stuff like this just take the abstract one?  This isn’t perf critical
code at all.