Bug 226200

Summary: Add the ability to dump statistics about cumulative counts and code sizes of Baseline JIT opcodes and DFG nodes
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
rmorisset: review+, ews-feeder: commit-queue-
patch for landing
ews-feeder: commit-queue-
patch for landing
none
[fast-cq] patch for landing none

Description Saam Barati 2021-05-24 17:29:53 PDT
...
Comment 1 Saam Barati 2021-05-26 14:53:44 PDT
Created attachment 429799 [details]
patch
Comment 2 Saam Barati 2021-05-26 15:04:03 PDT
Created attachment 429800 [details]
patch
Comment 3 Robin Morisset 2021-05-26 15:12:21 PDT
Comment on attachment 429799 [details]
patch

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

r=me with some comments (and once it applies cleanly on ToT)

> Source/JavaScriptCore/jit/JITSizeStatistics.cpp:64
> +    WTF::bubbleSort(entries.begin(), entries.end(), [] (auto lhs, auto rhs) {

Why bubbleSort here instead of std::sort? I see no reason to expect the entries to be nearly sorted.

> Source/JavaScriptCore/runtime/Options.cpp:573
> +        Options::useJITSizeStatistics() = true;

I am a bit surprised by this Options::useJITSizeStatistics(), was there a reason to make it an option (which can get incoherent) instead of just checking for "Options::dumpBaselineJITSizeStatistics() || Options::dumpDFGJITSizeStatistics()" in VM::VM() ? This seems to be the only place where it is used.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:3781
> +    addFunction(vm, "jitSizeStatistics", functionJITSizeStatistics, 1);

Maybe also adding a resetJitSizeStatistics might be worth doing, so that we could bracket each sub-benchmark in large benchmarks like JS2 by resetJitSizeStatistics / dumpJITSizeStatistics.
Comment 4 Mark Lam 2021-05-26 16:12:33 PDT
Comment on attachment 429800 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:414
> +        if (Options::dumpDFGJITSizeStatistics()) {

UNLIKELY().

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:427
> +        if (sizeMarker)

Ditto: UNLIKELY.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:440
> +        if (sizeMarker)

Ditto: UNLIKELY.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2112
> +        if (Options::dumpDFGJITSizeStatistics()) {

Ditto: UNLIKELY.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2119
> +        if (sizeMarker)

Ditto: UNLIKELY.

> Source/JavaScriptCore/jit/JIT.cpp:264
> +        if (m_bytecodeIndex >= startBytecodeIndex && Options::dumpBaselineJITSizeStatistics()) {

Ditto: UNLIKELY.

> Source/JavaScriptCore/jit/JIT.cpp:491
> +        if (sizeMarker)

Ditto: UNLIKELY.

> Source/JavaScriptCore/jit/JIT.cpp:547
> +        if (Options::dumpBaselineJITSizeStatistics()) {

Ditto: UNLIKELY.

> Source/JavaScriptCore/jit/JIT.cpp:664
> +        if (sizeMarker)

Ditto: UNLIKELY.

> Source/JavaScriptCore/jit/JIT.cpp:737
> +    if (Options::dumpBaselineJITSizeStatistics()) {

Ditto: UNLIKELY.

> Source/JavaScriptCore/jit/JIT.cpp:794
> +    if (sizeMarker)
> +        m_vm->jitSizeStatistics->markEnd(WTFMove(*sizeMarker), *this);

Ditto: UNLIKELY.

Note: you're missing the arityCheck blob of the header below (which I will be removing shortly though).  So, I don't care all that much about it.

> Source/JavaScriptCore/tools/JSDollarVM.cpp:3782
> +    addFunction(vm, "jitSizeStatistics", functionJITSizeStatistics, 1);
> +    addFunction(vm, "dumpJITSizeStatistics", functionDumpJITSizeStatistics, 1);

I think these take 0 arguments, not 1.
Comment 5 Robin Morisset 2021-05-26 16:33:32 PDT
Comment on attachment 429800 [details]
patch

r=me with the comments from before, the suggestions from Mark, and fixing the EWS compilation error.

And using std::sort with ">" in the lambda instead of ">="
Comment 6 Saam Barati 2021-05-27 14:58:03 PDT
Created attachment 429933 [details]
patch for landing
Comment 7 Saam Barati 2021-05-27 15:37:16 PDT
Created attachment 429941 [details]
patch for landing
Comment 8 Saam Barati 2021-05-28 11:32:30 PDT
Created attachment 430031 [details]
[fast-cq] patch for landing
Comment 9 EWS 2021-05-28 11:36:51 PDT
Committed r278213 (238255@main): <https://commits.webkit.org/238255@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430031 [details].
Comment 10 Radar WebKit Bug Importer 2021-05-28 11:37:23 PDT
<rdar://problem/78626317>