Summary: | JSC should be able to report profiling data associated with the IR dumps and disassembly | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | barraclough, benjamin, cdumez, dglazkov, ggaren, gyuyoung.kim, hnandor, kilvadyb, mark.lam, mhahnenberg, msaboff, ojan, oliver, ossy, peter+ews, philn, rakuco, sam, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Bug Depends on: | 103000, 103009, 103492, 103623, 103832, 104031, 104103, 104106 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2012-11-21 18:14:51 PST
Created attachment 177050 [details]
work in progress
Created attachment 177062 [details]
moar
Created attachment 177117 [details]
git-r-done!
Still some more stuff to do.
Created attachment 177119 [details]
almost there!
Created attachment 177127 [details]
sample profiling session
Sample session using the new display-pbp-output tool.
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.
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 on attachment 177128 [details] it does things Attachment 177128 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15105021 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 on attachment 177128 [details] it does things Attachment 177128 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15060933 Nice. 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 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 Created attachment 177134 [details]
WRONG PATCH
Comment on attachment 177134 [details]
WRONG PATCH
Not sure what I was doing there. Wrong patch.
Created attachment 177135 [details]
the patch
The right patch this time.
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.
(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 on attachment 177135 [details] the patch Attachment 177135 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15099110 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 on attachment 177135 [details] the patch Attachment 177135 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15060979 Created attachment 177136 [details]
the patch
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 on attachment 177136 [details] the patch Attachment 177136 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15106034 Comment on attachment 177136 [details] the patch Attachment 177136 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15100120 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 on attachment 177136 [details] the patch Attachment 177136 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15086545 Wow, you managed to break everything :-O 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). Created attachment 177152 [details]
try to break fewer things
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 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 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 Created attachment 177154 [details]
fix more bots
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 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.
(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. (In reply to comment #37) Yep, that all sounds good to me. (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 Landed in http://trac.webkit.org/changeset/136601 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 (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. CC the MIPS maintainer. I think we should track it in a separated bug. And it broke the ARM traditional build too - https://bugs.webkit.org/show_bug.cgi?id=104103 (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. |