Bug 217274 – Implement a GC verifier.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

rdar://55236585

[Mark Lam]

Snag #1
=======
I'm trying to templatize visitChildren() so that I can call it with 2 different kinds of visitors.  To minimize the amount of boiler plate that I have to write, I made ClassInfo's MethodTable refer to the instantiations of the visitChildren() templates.

Some classes like JSObject and JSGlobalObject need to export their visitChildren().  To make this work, I tried exporting an explicit instantiation of those template methods.

Unfortunately, this resulted in the explicitly instantiated template methods showing up as weak external symbols in the JavaScriptCore shared object.  This, in turn, results in a build failure because Tools/Scripts/check-for-weak-vtables-and-externals forbids weak external symbols in our built shared objects.

Now investigating to see if there's a way around this, or if I have to go the route of writing a lot of boilerplate code.

[Mark Lam]

Created attachment 410733 [details]
work in progress.

[Mark Lam]

A Clang engineer tells me that "there’s no way to export an explicit instantiation that will make it a strong symbol."  This is because "C++ does not provide any standard way to guarantee that an explicit instantiation is unique, and Clang hasn’t added any extension to do so."

So, I'm moving forward with adding a lot of boilerplate code (using macros as much as I can).

[Mark Lam]

Created attachment 410812 [details]
work in progress.

[Mark Lam]

Created attachment 410813 [details]
work in progress.

[Mark Lam]

Created attachment 410879 [details]
work in progress.

[Mark Lam]

Created attachment 410885 [details]
test patch to check EWS.

[Mark Lam]

Created attachment 417475 [details]
work in progress.

[Mark Lam]

Created attachment 417561 [details]
work in progress.

[Mark Lam]

Created attachment 417566 [details]
work in progress.

[Mark Lam]

Created attachment 417671 [details]
work in progress.

[Mark Lam]

rdar://56255683

[Mark Lam]

Created attachment 418900 [details]
work in progress.

[Mark Lam]

Created attachment 418958 [details]
work in progress.

It builds, runs, and crashes.

[Mark Lam]

Created attachment 419038 [details]
work in progress.

[Mark Lam]

Created attachment 419640 [details]
work in progress.

[Mark Lam]

Created attachment 419917 [details]
dirty patch with lots of debugging instrumentations & refactorings.

[Mark Lam]

Created attachment 419950 [details]
dirty patch with lots of debugging instrumentations & refactorings.

[Mark Lam]

Created attachment 419968 [details]
work in progress.

[Mark Lam]

Created attachment 420099 [details]
work in progress (clean).

[Mark Lam]

Created attachment 420209 [details]
work in progress.

[Mark Lam]

Created attachment 420237 [details]
work in progress.

[Mark Lam]

Created attachment 420245 [details]
work in progress.

[Mark Lam]

Created attachment 420246 [details]
work in progress.

[Mark Lam]

Created attachment 420274 [details]
work in progress.

[Mark Lam]

Created attachment 420297 [details]
work in progress.

[Mark Lam]

Created attachment 420450 [details]
work in progress.

[Mark Lam]

Created attachment 420455 [details]
work in progress.

[Mark Lam]

Created attachment 420511 [details]
work in progress.

[Mark Lam]

Created attachment 420516 [details]
work in progress.

[Mark Lam]

Created attachment 420519 [details]
work in progress.

[Mark Lam]

Created attachment 420675 [details]
work in progress.

[Mark Lam]

Created attachment 420680 [details]
test with verifyGC ON.

[Mark Lam]

Win EWS failures are not related.

[Mark Lam]

Created attachment 420899 [details]
proposed patch.

[Mark Lam]

Created attachment 420900 [details]
proposed patch.

[Filip Pizlo]

Comment on attachment 420900 [details]
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.

[Mark Lam]

Comment on attachment 420900 [details]
proposed patch.

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

Thanks for the review.

>> 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.

Yes, it may be true that this can remain a virtual function without a perf regression.  However, I did not initially aggressively templatize functions, and several rounds of benchmarking (on multiple device types) showed that there's some performance regression somewhere that is very hard to track down.  Since each round of benchmarking is very time consuming, I decided to just get aggressive on templatizing functions unless I know for sure that it will not benefit.  The plan is to eventually claw these back if additional benchmarking shows no regression going with a virtual dispatch.

For that reason, I'll land this patch with this level of templatization for now.

>> 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?

Initial benchmarking showed that we needed to templatize some constraints execution.  The interface for m_constraintSet->add() was changed to take a MarkingConstraintExecutorPair, and this one went along for the ride.  It is not clear to me if this specific constraint needed it or not.  scanExternalRememberedSet() probably does not, but it is less clear to me whether the other things visited in this constraint does or not.  I can revisit this later when I explore clawing back templates.

>> 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.

Will fix.

>> 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.

It does just take the abstract one.  Note: it's not templatized, and the new code only takes an AbstractSlotVisitor.

[Saam Barati]

Comment on attachment 420900 [details]
proposed patch.

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

Nice!! LGTM too

> Source/JavaScriptCore/ChangeLog:97
> +           we introduce some macros in SlotVisitorMacros.h to make reduce some of the

make reduce => reduce

> Source/JavaScriptCore/ChangeLog:110
> +           JSC_verifyGC and JSC_verboseVerifyGC.

nit: maybe a more canonical naming would be "useGCVerifier"?

> Source/JavaScriptCore/ChangeLog:209
> +            To improve the verifier making performance, we reserve a void* m_verifierMemo

making => marking

> Source/JavaScriptCore/API/JSCallbackObject.h:245
> +template <class Parent>
> +template<typename Visitor>

nit: put these on the  same template?
template <class Parent, typename Visitor>?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1110
> +// ALWAYS_INLINE because this only has 1 client.

nit: might not be necessary to say this. If the status quo changes, there will be a linker error

> Source/JavaScriptCore/heap/Heap.cpp:2831
> +                serviceSamplingProfiler(m_vm, visitor);

nit: I'd call this visitSamplingProfiler  I think

> Source/JavaScriptCore/heap/Heap.cpp:3072
> +void Heap::verifyGC()

👍

> Source/JavaScriptCore/heap/MarkStackMergingConstraint.cpp:64
> +    //    Hence, we can ignore thse stacks.

thse -> these

> Source/JavaScriptCore/runtime/StackFrame.h:62
> +    void visitChildren(Visitor& visitor)

nit: we should'v called this visitAggregate previously to go w/  our naming convention. Maybe worth doing

> Source/JavaScriptCore/wasm/WasmGlobal.h:60
> +//    template<typename Visitor> void visitAggregate(Visitor&);

oops

[Mark Lam]

(In reply to Saam Barati from comment #39)
> Comment on attachment 420900 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420900&action=review
> 
> Nice!! LGTM too

Thanks.

> > Source/JavaScriptCore/ChangeLog:97
> > +           we introduce some macros in SlotVisitorMacros.h to make reduce some of the
> 
> make reduce => reduce

Fixed.

> > Source/JavaScriptCore/ChangeLog:110
> > +           JSC_verifyGC and JSC_verboseVerifyGC.
> 
> nit: maybe a more canonical naming would be "useGCVerifier"?

I'll stick with JSC_verifyGC and JSC_verboseVerifyGC.  It's shorter and there is precedence: --verifyHeap, --verboseOSR.

> > Source/JavaScriptCore/ChangeLog:209
> > +            To improve the verifier making performance, we reserve a void* m_verifierMemo
> 
> making => marking

Fixed.

> > Source/JavaScriptCore/API/JSCallbackObject.h:245
> > +template <class Parent>
> > +template<typename Visitor>
> 
> nit: put these on the  same template?
> template <class Parent, typename Visitor>?

No, we can't do that.  JSCallbackObject is a template class parameterized on Parent, while the visitChildren method is a template method parameterized on Visitor within JSCallbackObject.  Hence, it has to be structured this way.

> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1110
> > +// ALWAYS_INLINE because this only has 1 client.
> 
> nit: might not be necessary to say this. If the status quo changes, there
> will be a linker error

I'll remove the comment.

> > Source/JavaScriptCore/heap/Heap.cpp:2831
> > +                serviceSamplingProfiler(m_vm, visitor);
> 
> nit: I'd call this visitSamplingProfiler  I think

Fixed.

> > Source/JavaScriptCore/heap/MarkStackMergingConstraint.cpp:64
> > +    //    Hence, we can ignore thse stacks.
> 
> thse -> these

Fixed.

> > Source/JavaScriptCore/runtime/StackFrame.h:62
> > +    void visitChildren(Visitor& visitor)
> 
> nit: we should'v called this visitAggregate previously to go w/  our naming
> convention. Maybe worth doing

Fixed.

> > Source/JavaScriptCore/wasm/WasmGlobal.h:60
> > +//    template<typename Visitor> void visitAggregate(Visitor&);
> 
> oops

Fixed.

[Mark Lam]

Landed in r273138: <http://trac.webkit.org/r273138>.