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

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren 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 50918: Supports OProfile JIT agent
https://bugs.webkit.org/attachment.cgi?id=50918&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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-.