[Webkit-unassigned] [Bug 32561] Provide a OProfile JIT agent (opagent) for the code generated by the JIT

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 1 12:16:25 PDT 2010


Geoffrey Garen <ggaren at apple.com> changed:

           What    |Removed                     |Added
  Attachment #50918|review?                     |review-
               Flag|                            |

--- Comment #6 from Geoffrey Garen <ggaren at apple.com>  2010-04-01 12:16:24 PST ---
(From update of attachment 50918)
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.

+#define oprofile(a) a
+#define oprofile(a)

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

+void JIT::emit_op_oprofile(Instruction* currentInstruction)
+    m_lineInfos.append(LineInfoRecord(size(),

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,
     m_jitCode = JIT::compile(scopeChainNode->globalData, codeBlock);

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>

Why does this need to be a SegmentedVector instead of just a Vector?

I think there enough to refine here to warrant an r-.

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list