Bug 102999 - JSC should be able to report profiling data associated with the IR dumps and disassembly
Summary: JSC should be able to report profiling data associated with the IR dumps and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on: 103000 103009 103492 103623 103832 104031 104103 104106
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-21 18:14 PST by Filip Pizlo
Modified: 2012-12-05 09:42 PST (History)
21 users (show)

See Also:


Attachments
work in progress (36.31 KB, patch)
2012-11-30 16:41 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
moar (116.11 KB, patch)
2012-11-30 17:23 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
git-r-done! (153.95 KB, patch)
2012-12-01 16:13 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
almost there! (163.39 KB, patch)
2012-12-01 17:10 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
sample profiling session (88.29 KB, text/plain)
2012-12-01 21:28 PST, Filip Pizlo
no flags Details
it does things (205.49 KB, patch)
2012-12-01 21:41 PST, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
WRONG PATCH (82.82 KB, patch)
2012-12-02 01:09 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (129.05 KB, patch)
2012-12-02 01:11 PST, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
the patch (129.06 KB, patch)
2012-12-02 01:33 PST, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
try to break fewer things (129.09 KB, patch)
2012-12-02 14:13 PST, Filip Pizlo
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
fix more bots (131.16 KB, patch)
2012-12-02 15:05 PST, Filip Pizlo
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-11-21 18:14:51 PST
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.
Comment 1 Filip Pizlo 2012-11-30 16:41:18 PST
Created attachment 177050 [details]
work in progress
Comment 2 Filip Pizlo 2012-11-30 17:23:59 PST
Created attachment 177062 [details]
moar
Comment 3 Filip Pizlo 2012-12-01 16:13:28 PST
Created attachment 177117 [details]
git-r-done!

Still some more stuff to do.
Comment 4 Filip Pizlo 2012-12-01 17:10:07 PST
Created attachment 177119 [details]
almost there!
Comment 5 Filip Pizlo 2012-12-01 21:28:53 PST
Created attachment 177127 [details]
sample profiling session

Sample session using the new display-pbp-output tool.
Comment 6 Filip Pizlo 2012-12-01 21:41:09 PST
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.
Comment 7 WebKit Review Bot 2012-12-01 21:46:09 PST
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.
Comment 8 Early Warning System Bot 2012-12-01 21:50:36 PST
Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15105021
Comment 9 EFL EWS Bot 2012-12-01 21:51:37 PST
Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15105020
Comment 10 Early Warning System Bot 2012-12-01 21:51:45 PST
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
Comment 11 Sam Weinig 2012-12-01 21:54:50 PST
Nice.
Comment 12 Build Bot 2012-12-01 22:08:49 PST
Comment on attachment 177128 [details]
it does things

Attachment 177128 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15064920
Comment 13 Peter Beverloo (cr-android ews) 2012-12-01 22:29:46 PST
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
Comment 14 Filip Pizlo 2012-12-02 01:09:20 PST
Created attachment 177134 [details]
WRONG PATCH
Comment 15 Filip Pizlo 2012-12-02 01:09:54 PST
Comment on attachment 177134 [details]
WRONG PATCH

Not sure what I was doing there.  Wrong patch.
Comment 16 Filip Pizlo 2012-12-02 01:11:00 PST
Created attachment 177135 [details]
the patch

The right patch this time.
Comment 17 WebKit Review Bot 2012-12-02 01:16:50 PST
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.
Comment 18 Filip Pizlo 2012-12-02 01:19:14 PST
(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.
Comment 19 Early Warning System Bot 2012-12-02 01:23:13 PST
Comment on attachment 177135 [details]
the patch

Attachment 177135 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15099110
Comment 20 Early Warning System Bot 2012-12-02 01:25:03 PST
Comment on attachment 177135 [details]
the patch

Attachment 177135 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15065926
Comment 21 EFL EWS Bot 2012-12-02 01:25:52 PST
Comment on attachment 177135 [details]
the patch

Attachment 177135 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15060979
Comment 22 Filip Pizlo 2012-12-02 01:33:35 PST
Created attachment 177136 [details]
the patch
Comment 23 WebKit Review Bot 2012-12-02 01:39:45 PST
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.
Comment 24 Early Warning System Bot 2012-12-02 01:46:02 PST
Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15106034
Comment 25 EFL EWS Bot 2012-12-02 01:46:15 PST
Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15100120
Comment 26 Early Warning System Bot 2012-12-02 01:47:24 PST
Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15064964
Comment 27 Build Bot 2012-12-02 02:19:05 PST
Comment on attachment 177136 [details]
the patch

Attachment 177136 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15086545
Comment 28 Oliver Hunt 2012-12-02 11:09:35 PST
Wow, you managed to break everything :-O
Comment 29 Chris Dumez 2012-12-02 11:14:44 PST
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).
Comment 30 Filip Pizlo 2012-12-02 14:13:26 PST
Created attachment 177152 [details]
try to break fewer things
Comment 31 WebKit Review Bot 2012-12-02 14:18:17 PST
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.
Comment 32 EFL EWS Bot 2012-12-02 14:52:56 PST
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
Comment 33 Build Bot 2012-12-02 14:56:08 PST
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
Comment 34 Filip Pizlo 2012-12-02 15:05:10 PST
Created attachment 177154 [details]
fix more bots
Comment 35 WebKit Review Bot 2012-12-02 15:09:01 PST
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.
Comment 36 Gavin Barraclough 2012-12-03 23:18:56 PST
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.
Comment 37 Filip Pizlo 2012-12-03 23:28:03 PST
(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.
Comment 38 Gavin Barraclough 2012-12-03 23:45:52 PST
(In reply to comment #37)

Yep, that all sounds good to me.
Comment 39 Filip Pizlo 2012-12-04 13:16:16 PST
(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
Comment 40 Filip Pizlo 2012-12-04 17:27:57 PST
Landed in http://trac.webkit.org/changeset/136601
Comment 41 Nandor Huszka 2012-12-04 23:17:09 PST
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
Comment 42 Filip Pizlo 2012-12-05 00:20:38 PST
(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.
Comment 43 Csaba Osztrogonác 2012-12-05 01:49:04 PST
CC the MIPS maintainer. I think we should track it in a separated bug.
Comment 44 Csaba Osztrogonác 2012-12-05 02:03:08 PST
And it broke the ARM traditional build too - https://bugs.webkit.org/show_bug.cgi?id=104103
Comment 45 Balazs Kilvady 2012-12-05 02:29:46 PST
(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.