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: | JavaScriptCore | Assignee: | 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
Saam Barati
2021-05-24 17:29:53 PDT
Created attachment 429799 [details]
patch
Created attachment 429800 [details]
patch
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 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 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 ">="
Created attachment 429933 [details]
patch for landing
Created attachment 429941 [details]
patch for landing
Created attachment 430031 [details]
[fast-cq] patch for landing
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]. |