The interface described here http://oprofile.sourceforge.net/doc/devel/jit-api.html should be implemented. This will allow to see the function names in the system profile.
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 build on win: Build output: http://queues.webkit.org/results/6768083
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