Bug 32561 – Provide a OProfile JIT agent (opagent) for the code generated by the JIT

• Component: JavaScriptCore
• Status: NEW

[Holger Freyther]

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.

[Holger Freyther]

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

[Zoltan Herczeg]

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().

[Gabor Loki]

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.

[Gabor Loki]

Created attachment 50919 [details]
sample report output

[Gabor Loki]

Created attachment 50920 [details]
sample annotation

[Geoffrey Garen]

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-.

[Gabor Loki]

> * 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!

[Holger Freyther]

(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.

[Gabor Loki]

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.

[Eric Seidel (no email)]

Attachment 53530 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1626505

[Gabor Loki]

> 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?

[Gabor Loki]

Created attachment 53661 [details]
Supports OProfile JIT agent

Unused parameter warning is fixed.

[Eric Seidel (no email)]

Attachment 53661 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1740058

[Gabor Loki]

Created attachment 53784 [details]
Supports OProfile JIT agent

New files are added to Xcode project as well.

[Eric Seidel (no email)]

Attachment 53784 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1749023

[Gabor Loki]

Created attachment 53793 [details]
Supports OProfile JIT agent

Err, it looks like that the Private flag was missing at JITProfileAgent.h.

[Nikolas Zimmermann]

Can any of the JSC hackers have a look? This is pending since months!

[Gavin Barraclough]

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.

[Xan Lopez]

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 :)

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 75792 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6780078

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Build Bot]

Attachment 75792 [details] did not build on win:
Build output: http://queues.webkit.org/results/6768083

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Attachment 75792 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6736095

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 75792 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6950045

[Xan Lopez]

Any chance to have a look at the general direction of the code before this bitrots too much?

[Gabor Loki]

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.

[Xan Lopez]

(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!

[Xan Lopez]

Created attachment 81054 [details]
jitprofile.diff

This should build at least in GTK+ and Qt.

[WebKit Review Bot]

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.

[Xan Lopez]

Created attachment 81055 [details]
jitprofile.diff

Better chances if I don't forget files.

[WebKit Review Bot]

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.

[Collabora GTK+ EWS bot]

Attachment 81054 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7689547

[Early Warning System Bot]

Attachment 81054 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7687873

[Build Bot]

Attachment 81055 [details] did not build on win:
Build output: http://queues.webkit.org/results/7691344

[Build Bot]

Attachment 81054 [details] did not build on win:
Build output: http://queues.webkit.org/results/7691346

[Early Warning System Bot]

Attachment 81055 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7690447

[Gustavo Noronha (kov)]

Attachment 81055 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7695173

[Collabora GTK+ EWS bot]

Attachment 81055 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7690472

[Xan Lopez]

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.

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

Attachment 81063 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7695179

[Xan Lopez]

Created attachment 81066 [details]
jitprofile.diff

C'mon.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 81063 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7693335

[Build Bot]

Attachment 81063 [details] did not build on win:
Build output: http://queues.webkit.org/results/7690480

[WebKit Review Bot]

Attachment 81055 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7691392

[Build Bot]

Attachment 81066 [details] did not build on win:
Build output: http://queues.webkit.org/results/7692316

[Early Warning System Bot]

Attachment 81066 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7694283

[Gustavo Noronha (kov)]

Attachment 81066 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7693349

[Xan Lopez]

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.

[Build Bot]

Attachment 81079 [details] did not build on win:
Build output: http://queues.webkit.org/results/7690507

[Collabora GTK+ EWS bot]

Attachment 81066 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7687929

[Xan Lopez]

Created attachment 81092 [details]
jitprofile.diff

This one has the XCode stuff, thanks to Holger.

[Build Bot]

Attachment 81092 [details] did not build on win:
Build output: http://queues.webkit.org/results/7698030

[WebKit Review Bot]

Attachment 81092 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7691603

[Oliver Hunt]

Hmmm, xan any progress on this?

[Xan Lopez]

(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-.

[Oliver Hunt]

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