JSC profiler should not count executions of op_call_put_result because doing so chang...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2012 19:18:52 +0000 (19:18 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2012 19:18:52 +0000 (19:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104102

Reviewed by Oliver Hunt.

Source/JavaScriptCore:

This removes op_call_put_result from profiling, since profiling it has an effect on
codegen. This fix enables all of SunSpider, V8, and Kraken to be profiled with the
new profiler.

To make this all fit together, the profiler now also reports in its output the exact
bytecode opcode name for each instruction (in addition to the stringified dump of that
bytecode), so that tools that grok the output can take note of op_call_put_result and
work around the fact that it has no counts.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
(JSC::DFG::ByteCodeParser::parseCodeBlock):
* dfg/DFGDriver.cpp:
(JSC::DFG::compile):
* jit/JIT.cpp:
(JSC::JIT::privateCompileMainPass):
* profiler/ProfilerBytecode.cpp:
(JSC::Profiler::Bytecode::toJS):
* profiler/ProfilerBytecode.h:
(JSC::Profiler::Bytecode::Bytecode):
(JSC::Profiler::Bytecode::opcodeID):
(Bytecode):
* profiler/ProfilerDatabase.cpp:
(JSC::Profiler::Database::ensureBytecodesFor):
* runtime/CommonIdentifiers.h:

Tools:

Modify the profiler to not output counts for op_call_put_result, since there
won't be any. Also fix a few weird bugs, like providing better error reporting
when you type something incorrectly and not reporting counts for slow paths
in the old JIT since those counts are actually not what you think they are
(we don't actually count slow path executions separately).

* Scripts/display-profiler-output:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@136720 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGDriver.cpp
Source/JavaScriptCore/jit/JIT.cpp
Source/JavaScriptCore/profiler/ProfilerBytecode.cpp
Source/JavaScriptCore/profiler/ProfilerBytecode.h
Source/JavaScriptCore/profiler/ProfilerDatabase.cpp
Source/JavaScriptCore/runtime/CommonIdentifiers.h
Tools/ChangeLog
Tools/Scripts/display-profiler-output

index 11d16e6..55c5b5a 100644 (file)
@@ -1,3 +1,36 @@
+2012-12-05  Filip Pizlo  <fpizlo@apple.com>
+
+        JSC profiler should not count executions of op_call_put_result because doing so changes DFG codegen
+        https://bugs.webkit.org/show_bug.cgi?id=104102
+
+        Reviewed by Oliver Hunt.
+
+        This removes op_call_put_result from profiling, since profiling it has an effect on
+        codegen. This fix enables all of SunSpider, V8, and Kraken to be profiled with the
+        new profiler.
+        
+        To make this all fit together, the profiler now also reports in its output the exact
+        bytecode opcode name for each instruction (in addition to the stringified dump of that
+        bytecode), so that tools that grok the output can take note of op_call_put_result and
+        work around the fact that it has no counts.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        (JSC::DFG::ByteCodeParser::parseCodeBlock):
+        * dfg/DFGDriver.cpp:
+        (JSC::DFG::compile):
+        * jit/JIT.cpp:
+        (JSC::JIT::privateCompileMainPass):
+        * profiler/ProfilerBytecode.cpp:
+        (JSC::Profiler::Bytecode::toJS):
+        * profiler/ProfilerBytecode.h:
+        (JSC::Profiler::Bytecode::Bytecode):
+        (JSC::Profiler::Bytecode::opcodeID):
+        (Bytecode):
+        * profiler/ProfilerDatabase.cpp:
+        (JSC::Profiler::Database::ensureBytecodesFor):
+        * runtime/CommonIdentifiers.h:
+
 2012-12-04  Filip Pizlo  <fpizlo@apple.com>
 
         display-profiler-output should be able to show source code
index 2d68c96..d860128 100644 (file)
@@ -2115,7 +2115,7 @@ bool ByteCodeParser::parseBlock(unsigned limit)
         m_currentInstruction = currentInstruction; // Some methods want to use this, and we'd rather not thread it through calls.
         OpcodeID opcodeID = interpreter->getOpcodeID(currentInstruction->u.opcode);
         
-        if (m_graph.m_compilation) {
+        if (m_graph.m_compilation && opcodeID != op_call_put_result) {
             addToGraph(CountExecution, OpInfo(m_graph.m_compilation->executionCounterFor(
                 Profiler::OriginStack(*m_globalData->m_perBytecodeProfiler, m_codeBlock, currentCodeOrigin()))));
         }
@@ -3714,6 +3714,12 @@ void ByteCodeParser::parseCodeBlock()
                     dataLogF("Creating basic block %p, #%zu for %p bc#%u at inline depth %u.\n", block.get(), m_graph.m_blocks.size(), m_inlineStackTop->executable(), m_currentIndex, CodeOrigin::inlineDepthForCallFrame(m_inlineStackTop->m_inlineCallFrame));
 #endif
                     m_currentBlock = block.get();
+                    // This assertion checks two things:
+                    // 1) If the bytecodeBegin is greater than currentIndex, then something has gone
+                    //    horribly wrong. So, we're probably generating incorrect code.
+                    // 2) If the bytecodeBegin is equal to the currentIndex, then we failed to do
+                    //    a peephole coalescing of this block in the if statement above. So, we're
+                    //    generating suboptimal code and leaving more work for the CFG simplifier.
                     ASSERT(m_inlineStackTop->m_unlinkedBlocks.isEmpty() || m_graph.m_blocks[m_inlineStackTop->m_unlinkedBlocks.last().m_blockIndex]->bytecodeBegin < m_currentIndex);
                     m_inlineStackTop->m_unlinkedBlocks.append(UnlinkedBlock(m_graph.m_blocks.size()));
                     m_inlineStackTop->m_blockLinkingTargets.append(m_graph.m_blocks.size());
index 8645c6d..42b5b79 100644 (file)
@@ -72,7 +72,7 @@ inline bool compile(CompileMode compileMode, ExecState* exec, CodeBlock* codeBlo
         return false;
     
 #if DFG_ENABLE(DEBUG_VERBOSE)
-    dataLogF("DFG compiling code block %p(%p) for executable %p, number of instructions = %u.\n", codeBlock, codeBlock->alternative(), codeBlock->ownerExecutable(), codeBlock->instructionCount());
+    dataLog("DFG compiling ", *codeBlock, ", number of instructions = ", codeBlock->instructionCount(), "\n");
 #endif
     
     // Derive our set of must-handle values. The compilation must be at least conservative
index 4eced2f..1592347 100644 (file)
@@ -234,14 +234,16 @@ void JIT::privateCompileMainPass()
         dataLogF("Old JIT emitting code for bc#%u at offset 0x%lx.\n", m_bytecodeOffset, (long)debugOffset());
 #endif
         
-        if (m_compilation) {
+        OpcodeID opcodeID = m_interpreter->getOpcodeID(currentInstruction->u.opcode);
+
+        if (m_compilation && opcodeID != op_call_put_result) {
             add64(
                 TrustedImm32(1),
                 AbsoluteAddress(m_compilation->executionCounterFor(Profiler::OriginStack(Profiler::Origin(
                     m_compilation->bytecodes(), m_bytecodeOffset)))->address()));
         }
 
-        switch (m_interpreter->getOpcodeID(currentInstruction->u.opcode)) {
+        switch (opcodeID) {
         DEFINE_BINARY_OP(op_del_by_val)
         DEFINE_BINARY_OP(op_in)
         DEFINE_BINARY_OP(op_less)
index 85bf8f2..2bc99b9 100644 (file)
@@ -34,6 +34,7 @@ JSValue Bytecode::toJS(ExecState* exec) const
 {
     JSObject* result = constructEmptyObject(exec);
     result->putDirect(exec->globalData(), exec->propertyNames().bytecodeIndex, jsNumber(m_bytecodeIndex));
+    result->putDirect(exec->globalData(), exec->propertyNames().opcode, jsString(exec, String::fromUTF8(opcodeNames[m_opcodeID])));
     result->putDirect(exec->globalData(), exec->propertyNames().description, jsString(exec, String::fromUTF8(m_description)));
     return result;
 }
index 5b563f6..57ae702 100644 (file)
@@ -27,6 +27,7 @@
 #define ProfilerBytecode_h
 
 #include "JSValue.h"
+#include "Opcode.h"
 #include <wtf/text/CString.h>
 
 namespace JSC { namespace Profiler {
@@ -38,18 +39,21 @@ public:
     {
     }
     
-    Bytecode(unsigned bytecodeIndex, const CString& description)
+    Bytecode(unsigned bytecodeIndex, OpcodeID opcodeID, const CString& description)
         : m_bytecodeIndex(bytecodeIndex)
+        , m_opcodeID(opcodeID)
         , m_description(description)
     {
     }
     
     unsigned bytecodeIndex() const { return m_bytecodeIndex; }
+    OpcodeID opcodeID() const { return m_opcodeID; }
     const CString& description() const { return m_description; }
     
     JSValue toJS(ExecState*) const;
 private:
     unsigned m_bytecodeIndex;
+    OpcodeID m_opcodeID;
     CString m_description;
 };
 
index 2cef7cd..2e20a7d 100644 (file)
@@ -80,7 +80,7 @@ Bytecodes* Database::ensureBytecodesFor(CodeBlock* codeBlock)
     for (unsigned bytecodeIndex = 0; bytecodeIndex < codeBlock->instructions().size();) {
         out.reset();
         codeBlock->dumpBytecode(out, bytecodeIndex);
-        result->append(Bytecode(bytecodeIndex, out.toCString()));
+        result->append(Bytecode(bytecodeIndex, m_globalData.interpreter->getOpcodeID(codeBlock->instructions()[bytecodeIndex].u.opcode), out.toCString()));
         
         bytecodeIndex += opcodeLength(
             m_globalData.interpreter->getOpcodeID(
index a9a43b9..91d2e08 100644 (file)
@@ -67,6 +67,7 @@
     macro(name) \
     macro(now) \
     macro(Object) \
+    macro(opcode) \
     macro(origin) \
     macro(parse) \
     macro(propertyIsEnumerable) \
index 9eb77cc..9e5f56e 100644 (file)
@@ -1,3 +1,18 @@
+2012-12-05  Filip Pizlo  <fpizlo@apple.com>
+
+        JSC profiler should not count executions of op_call_put_result because doing so changes DFG codegen
+        https://bugs.webkit.org/show_bug.cgi?id=104102
+
+        Reviewed by Oliver Hunt.
+
+        Modify the profiler to not output counts for op_call_put_result, since there
+        won't be any. Also fix a few weird bugs, like providing better error reporting
+        when you type something incorrectly and not reporting counts for slow paths
+        in the old JIT since those counts are actually not what you think they are
+        (we don't actually count slow path executions separately).
+
+        * Scripts/display-profiler-output:
+
 2012-12-05  Andras Becsi  <andras.becsi@digia.com>
 
         Fix compilation for Qt5.0.0 stable branch.
index 7e06bb1..c3a98bc 100755 (executable)
@@ -29,14 +29,19 @@ require 'json'
 require 'readline'
 
 class Bytecode
-    attr_accessor :bytecodeIndex, :description, :topCounts
+    attr_accessor :bytecodeIndex, :opcode, :description, :topCounts
     
-    def initialize(bytecodeIndex, description)
+    def initialize(bytecodeIndex, opcode, description)
         @bytecodeIndex = bytecodeIndex
+        @opcode = opcode
         @description = description
         @topCounts = []
     end
     
+    def shouldHaveCounts?
+        @opcode != "op_call_put_result"
+    end
+    
     def addTopCount(count)
         @topCounts << count
     end
@@ -73,7 +78,7 @@ class Bytecodes
         json["bytecode"].each {
             | subJson |
             index = subJson["bytecodeIndex"].to_i
-            @bytecode[index] = Bytecode.new(index, subJson["description"].to_s)
+            @bytecode[index] = Bytecode.new(index, subJson["opcode"].to_s, subJson["description"].to_s)
         }
     end
     
@@ -266,6 +271,11 @@ def executeCommand(*commandArray)
             return
         end
         
+        unless $engines.index(engine)
+            puts "#{engine} is not a valid engine, try #{$engines.join(' or ')}."
+            return
+        end
+        
         countCols = 10 * $engines.size
         $compilations.each {
             | compilation |
@@ -275,13 +285,15 @@ def executeCommand(*commandArray)
             puts(rpad($engines.join("/") + " Counts", countCols) + " Disassembly for #{hash} in #{engine}")
             compilation.descriptions.each {
                 | description |
-                next if description.description =~ /CountExecution\(/
-                if description.origin.empty?
+                # FIXME: We should have a better way of detecting things like CountExecution nodes
+                # and slow path entries in the baseline JIT.
+                next if description.description =~ /CountExecution\(/ and engine == "DFG"
+                if description.origin.empty? or not description.origin[-1].shouldHaveCounts? or (engine == "Baseline" and description.description =~ /^\s*\(S\)/)
                     countsString = ""
                 else
                     countsString = $engines.map {
-                        | engine |
-                        description.origin[-1].executionCount(engine)
+                        | myEngine |
+                        description.origin[-1].executionCount(myEngine)
                     }.join("/")
                 end
                 description.description.split("\n").each {