Bug 217274

Summary: Implement a GC verifier.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, annulen, beidson, benjamin, calvaris, cdumez, changseok, cmarcelo, dino, eric.carlson, esprehn+autocc, ews-watchlist, fpizlo, glenn, graouts, gyuyoung.kim, hi, jer.noble, joepeck, jsbell, kangil.han, keith_miller, kondapallykalyan, msaboff, philipj, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
work in progress.
none
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
test patch to check EWS.
none
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
work in progress.
none
work in progress.
none
work in progress.
ews-feeder: commit-queue-
work in progress.
none
work in progress.
none
dirty patch with lots of debugging instrumentations & refactorings.
none
dirty patch with lots of debugging instrumentations & refactorings.
none
work in progress.
ews-feeder: commit-queue-
work in progress (clean).
none
work in progress.
none
work in progress.
none
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
work in progress.
none
work in progress.
none
work in progress.
none
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
work in progress.
ews-feeder: commit-queue-
work in progress.
none
test with verifyGC ON.
ews-feeder: commit-queue-
proposed patch.
ews-feeder: commit-queue-
proposed patch. fpizlo: review+

Description Mark Lam 2020-10-02 20:50:50 PDT
rdar://55236585
Comment 1 Mark Lam 2020-10-06 21:20:02 PDT
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.
Comment 2 Mark Lam 2020-10-06 21:31:11 PDT
Created attachment 410733 [details]
work in progress.
Comment 3 Mark Lam 2020-10-07 23:30:13 PDT
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).
Comment 4 Mark Lam 2020-10-07 23:30:58 PDT
Created attachment 410812 [details]
work in progress.
Comment 5 Mark Lam 2020-10-07 23:32:58 PDT
Created attachment 410813 [details]
work in progress.
Comment 6 Mark Lam 2020-10-08 13:45:08 PDT
Created attachment 410879 [details]
work in progress.
Comment 7 Mark Lam 2020-10-08 14:27:44 PDT
Created attachment 410885 [details]
test patch to check EWS.
Comment 8 Mark Lam 2021-01-12 11:45:21 PST
Created attachment 417475 [details]
work in progress.
Comment 9 Mark Lam 2021-01-13 14:03:33 PST
Created attachment 417561 [details]
work in progress.
Comment 10 Mark Lam 2021-01-13 14:30:49 PST
Created attachment 417566 [details]
work in progress.
Comment 11 Mark Lam 2021-01-14 17:15:18 PST
Created attachment 417671 [details]
work in progress.
Comment 12 Mark Lam 2021-01-25 10:34:03 PST
rdar://56255683
Comment 13 Mark Lam 2021-02-01 11:11:37 PST
Created attachment 418900 [details]
work in progress.
Comment 14 Mark Lam 2021-02-01 21:53:52 PST
Created attachment 418958 [details]
work in progress.

It builds, runs, and crashes.
Comment 15 Mark Lam 2021-02-02 11:41:11 PST
Created attachment 419038 [details]
work in progress.
Comment 16 Mark Lam 2021-02-08 15:15:45 PST
Created attachment 419640 [details]
work in progress.
Comment 17 Mark Lam 2021-02-10 16:18:25 PST
Created attachment 419917 [details]
dirty patch with lots of debugging instrumentations & refactorings.
Comment 18 Mark Lam 2021-02-10 23:52:33 PST
Created attachment 419950 [details]
dirty patch with lots of debugging instrumentations & refactorings.
Comment 19 Mark Lam 2021-02-11 02:23:50 PST
Created attachment 419968 [details]
work in progress.
Comment 20 Mark Lam 2021-02-11 22:09:18 PST
Created attachment 420099 [details]
work in progress (clean).
Comment 21 Mark Lam 2021-02-12 22:25:59 PST
Created attachment 420209 [details]
work in progress.
Comment 22 Mark Lam 2021-02-14 01:19:08 PST
Created attachment 420237 [details]
work in progress.
Comment 23 Mark Lam 2021-02-14 11:54:38 PST
Created attachment 420245 [details]
work in progress.
Comment 24 Mark Lam 2021-02-14 12:03:21 PST
Created attachment 420246 [details]
work in progress.
Comment 25 Mark Lam 2021-02-14 23:54:04 PST
Created attachment 420274 [details]
work in progress.
Comment 26 Mark Lam 2021-02-15 03:05:05 PST
Created attachment 420297 [details]
work in progress.
Comment 27 Mark Lam 2021-02-16 04:36:04 PST
Created attachment 420450 [details]
work in progress.
Comment 28 Mark Lam 2021-02-16 05:17:44 PST
Created attachment 420455 [details]
work in progress.
Comment 29 Mark Lam 2021-02-16 11:42:22 PST
Created attachment 420511 [details]
work in progress.
Comment 30 Mark Lam 2021-02-16 12:00:18 PST
Created attachment 420516 [details]
work in progress.
Comment 31 Mark Lam 2021-02-16 12:23:46 PST
Created attachment 420519 [details]
work in progress.
Comment 32 Mark Lam 2021-02-17 11:11:11 PST
Created attachment 420675 [details]
work in progress.
Comment 33 Mark Lam 2021-02-17 11:18:52 PST
Created attachment 420680 [details]
test with verifyGC ON.
Comment 34 Mark Lam 2021-02-18 10:08:20 PST
Win EWS failures are not related.
Comment 35 Mark Lam 2021-02-18 18:44:44 PST
Created attachment 420899 [details]
proposed patch.
Comment 36 Mark Lam 2021-02-18 18:57:58 PST
Created attachment 420900 [details]
proposed patch.
Comment 37 Filip Pizlo 2021-02-18 20:02:02 PST
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.
Comment 38 Mark Lam 2021-02-18 20:54:10 PST
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.
Comment 39 Saam Barati 2021-02-18 22:23:44 PST
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
Comment 40 Mark Lam 2021-02-18 22:54:50 PST
(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.
Comment 41 Mark Lam 2021-02-19 07:53:16 PST
Landed in r273138: <http://trac.webkit.org/r273138>.