Bug 102999 – JSC should be able to report profiling data associated with the IR dumps and disassembly

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

[Filip Pizlo]

It would be really easy to do a per-bytecode-instruction counting profiler.  It would be only slightly harder to do a sampling profiler that does the same.  But right now reporting the results would be hard.

I want jsc to be able to dump profiling output, either periodically to the console, at the end of execution to the console, or to just internally as a JSON dump for the web inspector for example, that looks something like:

CodeBlock 0x6426426, instruction count = 52:
    source code:
           function foo(p, a, b) { if (p) return a + b; }
    profiled (LLInt/Baseline/DFG) bytecode:
           100/900/700     [  0] op_enter
           100/900/700     [  1] op_jeq_null
           75/800/500       [  5] op_add
           75/1300/0         [  9] op_ret
           25/100/200       [ 11] op_ret

Or something.

To do that I'll need the following:

- Everything that gets printed to the console for disassembly should have the option of being saved in memory (probably as a sequence of strings, one per bytecode instruction) until such time as we're ready to print it.  That means rethinking dataLog().
- Some better way of identifying (naming) a code block.  I'm really getting tired of the hex code block addresses and instruction counts.
- We need some counters somewhere and the data structures for storing the above.

[Filip Pizlo]

Created attachment 177050 [details]
work in progress

[Filip Pizlo]

Created attachment 177062 [details]
moar

[Filip Pizlo]

Created attachment 177117 [details]
git-r-done!

Still some more stuff to do.

[Filip Pizlo]

Created attachment 177119 [details]
almost there!

[Filip Pizlo]

Created attachment 177127 [details]
sample profiling session

Sample session using the new display-pbp-output tool.

[Filip Pizlo]

Created attachment 177128 [details]
it does things

I think this will probably still make EWS blow up.  Also, I may yet split the patch up a bit, though I have very little wiggle room in that department.

[WebKit Review Bot]

Attachment 177128 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/profiler/PBPOriginStack.h:50:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/profiler/PBPOriginStack.h:56:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/profiler/PBPBytecodes.h:38:  The parameter name "hash" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/profiler/PBPDatabase.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WTF/wtf/text/WTFString.h:443:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/profiler/PBPCompilationKind.h:31:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 6 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15105021

[EFL EWS Bot]

Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15105020

[Early Warning System Bot]

Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15060933

[Sam Weinig]

Nice.

[Build Bot]

Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15064920

[Peter Beverloo (cr-android ews)]

Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15100055

[Filip Pizlo]

Created attachment 177134 [details]
WRONG PATCH

[Filip Pizlo]

Comment on attachment 177134 [details]
WRONG PATCH

Not sure what I was doing there.  Wrong patch.

[Filip Pizlo]

Created attachment 177135 [details]
the patch

The right patch this time.

[WebKit Review Bot]

Attachment 177135 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/profiler/PBPCompilationKind.h:34:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #17)
> Attachment 177135 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
> Source/JavaScriptCore/profiler/PBPCompilationKind.h:34:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
> Total errors found: 1 in 60 files

I believe it's getting confused by "DFG" being an enum member.  I don't believe that "Dfg" would have been correct.

> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 177135 [details]
the patch

Attachment 177135 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15099110

[Early Warning System Bot]

Comment on attachment 177135 [details]
the patch

Attachment 177135 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15065926

[EFL EWS Bot]

Comment on attachment 177135 [details]
the patch

Attachment 177135 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15060979

[Filip Pizlo]

Created attachment 177136 [details]
the patch

[WebKit Review Bot]

Attachment 177136 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/profiler/PBPCompilationKind.h:34:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15106034

[EFL EWS Bot]

Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15100120

[Early Warning System Bot]

Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15064964

[Build Bot]

Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15086545

[Oliver Hunt]

Wow, you managed to break everything :-O

[Chris Dumez]

Comment on attachment 177136 [details]
the patch

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

> Source/JavaScriptCore/profiler/PBPBytecode.h:31
> +

#include <climits> is needed for UINT_MAX (is causing the failure on EFL port).

[Filip Pizlo]

Created attachment 177152 [details]
try to break fewer things

[WebKit Review Bot]

Attachment 177152 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/profiler/PBPCompilationKind.h:34:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 177152 [details]
try to break fewer things

Attachment 177152 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15105277

[Build Bot]

Comment on attachment 177152 [details]
try to break fewer things

Attachment 177152 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15121036

[Filip Pizlo]

Created attachment 177154 [details]
fix more bots

[WebKit Review Bot]

Attachment 177154 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/CMakeLists.txt', u'S..." exit_code: 1
Source/JavaScriptCore/profiler/PBPCompilationKind.h:34:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Comment on attachment 177154 [details]
fix more bots

I'm not wild about the name "PBP".  WebKit style is anti-acronyms (though I guess there is solid precedent for namespace names).
I'd consider thinking about a rename – maybe simply 'Profiler', or another whole-word name – I know we have a legacy profiling tool, but I don't think that's touched enough to be a practical concern.

[Filip Pizlo]

(In reply to comment #36)
> (From update of attachment 177154 [details])
> I'm not wild about the name "PBP".  WebKit style is anti-acronyms (though I guess there is solid precedent for namespace names).
> I'd consider thinking about a rename – maybe simply 'Profiler', or another whole-word name – I know we have a legacy profiling tool, but I don't think that's touched enough to be a practical concern.

I like Profiler.  I will rename the PBP namespace to Profiler.

But this raises the question of how to name files.  Currently all of the PBP files use the PBP prefix.  I propose simply prefixing the filenames with "Profiler".  This will be a bit verbose, i.e. we will have things like Source/JavaScriptCore/profiler/ProfilerDatabase.h.  But, I think it has the benefit of clarity.

I will also rename display-pbp-output to display-profiler-ouput.

Let me know if you think that's all reasonable.

[Gavin Barraclough]

(In reply to comment #37)

Yep, that all sounds good to me.

[Filip Pizlo]

(In reply to comment #38)
> (In reply to comment #37)
> 
> Yep, that all sounds good to me.

There is already a class called Profiler, which conflicts with the use of Profiler as a namespace.

I'm going to rename the Profiler class to LegacyProfiler: https://bugs.webkit.org/show_bug.cgi?id=104031

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/136601

[Nandor Huszka]

It broke the Qt build, fix it please.

/data/buildbot/mips-1/qt-linux-mipsel-mips32r2-release/build/Source/JavaScriptCore/jit/JIT.cpp: In member function 'void JSC::JIT::privateCompileMainPass()':
/data/buildbot/mips-1/qt-linux-mipsel-mips32r2-release/build/Source/JavaScriptCore/jit/JIT.cpp:241: error: 'add64' was not declared in this scope

[Filip Pizlo]

(In reply to comment #41)
> It broke the Qt build, fix it please.
> 
> /data/buildbot/mips-1/qt-linux-mipsel-mips32r2-release/build/Source/JavaScriptCore/jit/JIT.cpp: In member function 'void JSC::JIT::privateCompileMainPass()':
> /data/buildbot/mips-1/qt-linux-mipsel-mips32r2-release/build/Source/JavaScriptCore/jit/JIT.cpp:241: error: 'add64' was not declared in this scope

This is something that the MIPS maintainers should fix.

[Csaba Osztrogonác]

CC the MIPS maintainer. I think we should track it in a separated bug.

[Csaba Osztrogonác]

And it broke the ARM traditional build too - https://bugs.webkit.org/show_bug.cgi?id=104103

[Balazs Kilvady]

(In reply to comment #43)
> CC the MIPS maintainer. I think we should track it in a separated bug.

I added Bug 104106 for it.