[webkit-reviews] review denied: [Bug 32561] Provide a OProfile JIT agent (opagent) for the code generated by the JIT : [Attachment 53793] Supports OProfile JIT agent

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Gabor Loki
<loki at webkit.org>'s request for review:
Bug 32561: Provide a OProfile JIT agent (opagent) for the code generated by the
JIT
https://bugs.webkit.org/show_bug.cgi?id=32561

Attachment 53793: Supports OProfile JIT agent
https://bugs.webkit.org/attachment.cgi?id=53793&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
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.