Summary: | Provide a OProfile JIT agent (opagent) for the code generated by the JIT | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Holger Freyther <zecke> | ||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
Status: | NEW --- | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | barraclough, buildbot, eric, gustavo.noronha, gustavo, kent.hansen, laszlo.gombos, loki, mjs, oliver, webkit.review.bot, xan.lopez, zherczeg, zimmermann | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Holger Freyther
2009-12-15 08:28:09 PST
Created attachment 44882 [details]
Proposal for implementing the feature
This is the first compiling/linking version. At this point I would like to get feedback on:
- The places I have patched. Are they the right one?
- The way it is patched..
- The buildsystem change
- And if I should patch it in a similiar way for YARR (when JITTED)
What is not implemented:
- Using the information in the generateJITCode methods to have a proper symbol name
Hi, wow, seems rather easy to work with oprofile. > - The places I have patched. Are they the right one? I would consider to patch the platform dependent executableCopy(...) functions in JavaScriptCore/assembler directory, since all JIT code is allocated through this function. Although it does not provide information about the purpose of the JIT code. (whole function, common code (machine trampolines), property cache code fragment, etc.) > - The way it is patched.. Most of the code in JavaScriptCore refactored not to use plain void* pointers (Although I like void* pointers, since its purpose is clear). Perhaps passing a CodePtr (m_jitCode) would be the preferred way. > - The buildsystem change I am not an expert on this topic, so it is fine by me. > - And if I should patch it in a similiar way for YARR (when JITTED) Seems necessary if we don't want to change the executableCopy(). Created attachment 50918 [details]
Supports OProfile JIT agent
This patch adds OProfile support for JSC.
Now the JS source/url path, function names and line information are available through OProfile's opreport and opannotate tools.
Created attachment 50919 [details]
sample report output
Created attachment 50920 [details]
sample annotation
Comment on attachment 50918 [details]
Supports OProfile JIT agent
Looks like a really useful tool.
A few high-level comments:
* Can you only profile on Linux?
* Can we remove some of our other sampling mechanisms in favor of this?
* Is there a big advantage to making this a compile-time switch rather than a run-time switch?
* It would be nice if the interface to JITProfileAgent were a little less oprofile-specific, so it could be hooked up to other profiling back-ends without too much work.
It's not clear to me when you would enable OPROFILE_JIT_AGENT vs OPROFILE_JIT_AGENT_WITH_SAFE_UNLOAD. This is worth an explanation in a comment and in the ChangeLog.
+ void* vma()
+ {
+ return m_code;
+ }
You've duplicated the code() function. Also, the code function says, "Keep this private!" but you've made it public. That seems wrong.
You need to do something to ensure that no one can call this function while the LinkBuffer is in an invalid state.
+#if ENABLE(OPROFILE_JIT_AGENT)
+#define oprofile(a) a
+#else
+#define oprofile(a)
+#endif
+ oprofile(macro(op_oprofile, 3)) \
This is pretty clunky. Is there really such a big advantage to ensuring the op_oprofile opcode doesn't exist in non-oprofile builds? We don't go to the same lengths for things like op_debug.
+#if ENABLE(OPROFILE_JIT_AGENT)
+void JIT::emit_op_oprofile(Instruction* currentInstruction)
+{
+ m_lineInfos.append(LineInfoRecord(size(), currentInstruction[1].u.operand));
+}
+#endif
This function appears twice. It should appear only once.
Many instructions can span the same line. I think this function should coalesce adjacent instructions, to avoid constructing overly large data structures.
+ m_jitCode = JIT::compileAndAnnotate(scopeChainNode->globalData, codeBlock, symbolName);
+#else
m_jitCode = JIT::compile(scopeChainNode->globalData, codeBlock);
+#endif
I don't think there should be two different ways to compile unless they actually compile differently. If we want compilation to annotate, but only in oprofile builds, the hooks for annotation should go into JIT::compile.
+ // TODO: make it thread safe
The way to make this thread-safe is to make it a property of the JSGlobalData, rather than a global.
+ typedef SegmentedVector<VirtualMemorySegment, 256> VirtualMemorySegmentVector;
Why does this need to be a SegmentedVector instead of just a Vector?
I think there enough to refine here to warrant an r-.
> * Can you only profile on Linux? > * Can we remove some of our other sampling mechanisms in favor of this? I wish I could say yes to both questions, but OProfile is only for Linux. A kernel driver is needed at OS level. Probably it is not impossible to implement such a module on Mac as well ;) > * Is there a big advantage to making this a compile-time switch rather than a > run-time switch? There were very few samples for OProfile part of WebKit and its agent as well. So, we can use run-time option, but it should be kept in mind not to grow and slow down JSC with useless checks where OProfile is not supported. > It's not clear to me when you would enable OPROFILE_JIT_AGENT vs > OPROFILE_JIT_AGENT_WITH_SAFE_UNLOAD. This is worth an explanation in a comment > and in the ChangeLog. SAFE_UNLOAD should be enabled by default. If it is disabled there could be some misplaced samples, but JSC runs faster. > + void* vma() > + { > + return m_code; > + } > > You've duplicated the code() function. Also, the code function says, "Keep this private!" but you've made it public. That seems wrong. Probably we can use executableAddress(), but the m_size has to be published anyway. > You need to do something to ensure that no one can call this function while the LinkBuffer is in an invalid state. What do you mean about invalid state? The state of JIT code (executable or flushed) does not matter for OProfile. Only the memory range is needed. The profiler will not touch that memory range at all. Other things will be fixed. Thanks for your inputs! (In reply to comment #7) > > * Can you only profile on Linux? > > * Can we remove some of our other sampling mechanisms in favor of this? > > I wish I could say yes to both questions, but OProfile is only for Linux. A > kernel driver is needed at OS level. Probably it is not impossible to implement > such a module on Mac as well ;) The more natural choice on OS X would be to use DTrace but up to my knowledge the DTrace JIT support was never completed by SUN but we could look at the Java sourcecode to verify that. Created attachment 53530 [details]
Supports OProfile JIT agent
The patch is rewritten according to Geoffrey's comment, except one thing. It looks like that we cannot eliminate all JIT_PROFILE_AGENT macro, because there will be a small performance regression on ARM (~1-2%) if the agent is disabled.
Attachment 53530 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1626505 > Build output: http://webkit-commit-queue.appspot.com/results/1626505
Oh, I missed that unused parameter. I will fix it. Apart from that, how does this patch look like?
Created attachment 53661 [details]
Supports OProfile JIT agent
Unused parameter warning is fixed.
Attachment 53661 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1740058 Created attachment 53784 [details]
Supports OProfile JIT agent
New files are added to Xcode project as well.
Attachment 53784 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1749023 Created attachment 53793 [details]
Supports OProfile JIT agent
Err, it looks like that the Private flag was missing at JITProfileAgent.h.
Can any of the JSC hackers have a look? This is pending since months! Comment on attachment 53793 [details]
Supports OProfile JIT agent
I think this needs tidying up a bit to reduce the amount to code added outside of the profiler.
Using op_profile_jit to gather the line number information should not be necessary; this should already be available from the codeblock from m_lineInfo. If you can switch over to using the existing info it looks like the new op should be unnecessary.
YARR should not be calling into the JIT; these should be separate subsystems. We should probably create a new top level directory (say, tools), and add this there. (ooi ExecuableAllocator is also called from Yarr; this should not be in the jit directory).
The interpreter should not ASSERT_NOT_REACHED on the new op, if it remains – we now build both the interpreter and the JIT into the same build.
Adding work to non-profiling builds is a no-go; the changes in Executable need to be reverted. The executable can be accessed from the codeblock, so the codeblock could just be passed in to the profiling agent, and the string could be calculated there. The changes in JIT.h should be moved across into the agent; given the complexity of the JIT keeping the footprint of tools to the minimum will reduce complexity. The JIT should only make a single function call here, passing the codeblock from which the data can be pulled.
The other changes throughout the JIT should be reduced to a single line macro call (i.e. compiles out to nothing if profiler is not built in, so skip having the extra lines for the compile guard).
Instead of passing strings to label the code, enum values should be used (different profiling tools may need different formats for names).
Enough comments for now, will be happy to rereview an updated patch.
Created attachment 75792 [details]
jitoprofile.diff
This patch tries to address all the review points raised in the last comment. I'm particularly unsure of whether the way I'm using the CodeBlock stuff to get the line info is correct :)
Attachment 75792 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75792 [details] did not build on qt: Build output: http://queues.webkit.org/results/6780078 Attachment 75792 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75792 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75792 [details] did not build on win: Build output: http://queues.webkit.org/results/6768083 Attachment 75792 [details] did not pass style-queue:
Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75792 [details] did not build on mac: Build output: http://queues.webkit.org/results/6736095 Attachment 75792 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061
Died at WebKitTools/Scripts/update-webkit line 132.
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 75792 [details] did not build on gtk: Build output: http://queues.webkit.org/results/6950045 Any chance to have a look at the general direction of the code before this bitrots too much? Comment on attachment 75792 [details]
jitoprofile.diff
First of all thanks for the update, I am deeply obliged to you!
Unfortunately this patch does not build on many platforms. We should work further on the patch to fix them. So, I am going to set the r? to r- because of the builds.
In order to fix the Qt build you should add
* 'tools/JITProfileAgent.cpp \' line to the 'SOURCES +=' section in Source/JavaScriptCore/JavaScriptCore.pro
* and '$$PWD/tools \' line to to the 'JAVASCRIPTCORE_INCLUDEPATH =' section in
Source/JavaScriptCore/JavaScriptCore.pri
Xan, could you update the patch again (because of the top Source directory) ?
I feel there is another issue about the codeBlock->lineInfo() function. Unfortunately I had no time to debug the code, but I have tried the jsc binary on ARM and the annotation was failed. There was no or not enough valid lineinfo data in the saved jitdump file. The correlation between source annotation and profile report was not sufficient. Xan, is the source annotation working for you?
One more thing. You changed the name of registerJITCodeImpl function to recordJITCodeImpl, but you forgot to change the dummy one (when JIT_OPROFILE_AGENT==0) to the similar name.
(In reply to comment #30) > (From update of attachment 75792 [details]) > First of all thanks for the update, I am deeply obliged to you! > Unfortunately this patch does not build on many platforms. We should work further on the patch to fix them. So, I am going to set the r? to r- because of the builds. Hey, yeah, I was aware of this. I was basically interested in Gavin saying if my refactoring was along the lines of what he was asking for. In any case I'll try to get it building in all the EWS bots. > > In order to fix the Qt build you should add > * 'tools/JITProfileAgent.cpp \' line to the 'SOURCES +=' section in Source/JavaScriptCore/JavaScriptCore.pro > * and '$$PWD/tools \' line to to the 'JAVASCRIPTCORE_INCLUDEPATH =' section in > Source/JavaScriptCore/JavaScriptCore.pri > > Xan, could you update the patch again (because of the top Source directory) ? Yep, I've done this locally already, will upload the updated patch later today. > > I feel there is another issue about the codeBlock->lineInfo() function. Unfortunately I had no time to debug the code, but I have tried the jsc binary on ARM and the annotation was failed. There was no or not enough valid lineinfo data in the saved jitdump file. The correlation between source annotation and profile report was not sufficient. Xan, is the source annotation working for you? Yeah, it works as well as the original patch worked for me. Are you sure you are passing -p to the jsc binary? > > One more thing. You changed the name of registerJITCodeImpl function to recordJITCodeImpl, but you forgot to change the dummy one (when JIT_OPROFILE_AGENT==0) to the similar name. Yes, this is why it failed on GTK+ in EWS. I noticed it after uploading, it's also fixed locally. Thanks! Created attachment 81054 [details]
jitprofile.diff
This should build at least in GTK+ and Qt.
Attachment 81054 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Ja..." exit_code: 1
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:35: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 81055 [details]
jitprofile.diff
Better chances if I don't forget files.
Attachment 81055 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Ja..." exit_code: 1
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:35: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 81054 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7689547 Attachment 81054 [details] did not build on qt: Build output: http://queues.webkit.org/results/7687873 Attachment 81055 [details] did not build on win: Build output: http://queues.webkit.org/results/7691344 Attachment 81054 [details] did not build on win: Build output: http://queues.webkit.org/results/7691346 Attachment 81055 [details] did not build on qt: Build output: http://queues.webkit.org/results/7690447 Attachment 81055 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7695173 Attachment 81055 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7690472 Created attachment 81063 [details]
jitprofile.diff
This should fix GTK+ (different arch. than what I have at home). Not sure why Qt is not finding the header, can't find where I should add it.
Attachment 81063 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Ja..." exit_code: 1
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:35: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 81063 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7695179 Created attachment 81066 [details]
jitprofile.diff
C'mon.
Attachment 81066 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/Ja..." exit_code: 1
Source/JavaScriptCore/jit/JITPropertyAccess.cpp:35: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 81063 [details] did not build on qt: Build output: http://queues.webkit.org/results/7693335 Attachment 81063 [details] did not build on win: Build output: http://queues.webkit.org/results/7690480 Attachment 81055 [details] did not build on mac: Build output: http://queues.webkit.org/results/7691392 Attachment 81066 [details] did not build on win: Build output: http://queues.webkit.org/results/7692316 Attachment 81066 [details] did not build on qt: Build output: http://queues.webkit.org/results/7694283 Attachment 81066 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7693349 Created attachment 81079 [details]
jitprofile.diff
Qt/WebKit2 duplicates the JSC includes, so I had to add the tools/ dir there too. Hopefully it will work now.
Attachment 81079 [details] did not build on win: Build output: http://queues.webkit.org/results/7690507 Attachment 81066 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7687929 Created attachment 81092 [details]
jitprofile.diff
This one has the XCode stuff, thanks to Holger.
Attachment 81092 [details] did not build on win: Build output: http://queues.webkit.org/results/7698030 Attachment 81092 [details] did not build on mac: Build output: http://queues.webkit.org/results/7691603 Hmmm, xan any progress on this? (In reply to comment #60) > Hmmm, xan any progress on this? Well, obviously I failed in making it build on Mac/Win, but I think in general the patch could still see some review from Gavin, since I tried to address all his concerns in the last round. It surely will need to be updated at this point, though, so feel free to r-. Comment on attachment 81092 [details]
jitprofile.diff
Okay, i'll r- this. I think we really just want this to be set up like other paltform specific things, eg. a header that defines a standard API then FooOProfile.cpp, FooSharkIFTheyEverDoSomethingToHelpUs.cpp, etc
|