Bug 226200 – Add the ability to dump statistics about cumulative counts and code sizes of Baseline JIT opcodes and DFG nodes

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

[Saam Barati]

...

[Saam Barati]

Created attachment 429799 [details]
patch

[Saam Barati]

Created attachment 429800 [details]
patch

[Robin Morisset]

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.

[Mark Lam]

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.

[Robin Morisset]

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 ">="

[Saam Barati]

Created attachment 429933 [details]
patch for landing

[Saam Barati]

Created attachment 429941 [details]
patch for landing

[Saam Barati]

Created attachment 430031 [details]
[fast-cq] patch for landing

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/78626317>