- Revision
- 136720
- Author
- [email protected]
- Date
- 2012-12-05 11:18:52 -0800 (Wed, 05 Dec 2012)
Log Message
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.
Source/_javascript_Core:
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:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (136719 => 136720)
--- trunk/Source/_javascript_Core/ChangeLog 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-12-05 19:18:52 UTC (rev 136720)
@@ -1,3 +1,36 @@
+2012-12-05 Filip Pizlo <[email protected]>
+
+ 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 <[email protected]>
display-profiler-output should be able to show source code
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (136719 => 136720)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2012-12-05 19:18:52 UTC (rev 136720)
@@ -2115,7 +2115,7 @@
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 @@
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());
Modified: trunk/Source/_javascript_Core/dfg/DFGDriver.cpp (136719 => 136720)
--- trunk/Source/_javascript_Core/dfg/DFGDriver.cpp 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/dfg/DFGDriver.cpp 2012-12-05 19:18:52 UTC (rev 136720)
@@ -72,7 +72,7 @@
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
Modified: trunk/Source/_javascript_Core/jit/JIT.cpp (136719 => 136720)
--- trunk/Source/_javascript_Core/jit/JIT.cpp 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/jit/JIT.cpp 2012-12-05 19:18:52 UTC (rev 136720)
@@ -234,14 +234,16 @@
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)
Modified: trunk/Source/_javascript_Core/profiler/ProfilerBytecode.cpp (136719 => 136720)
--- trunk/Source/_javascript_Core/profiler/ProfilerBytecode.cpp 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/profiler/ProfilerBytecode.cpp 2012-12-05 19:18:52 UTC (rev 136720)
@@ -34,6 +34,7 @@
{
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;
}
Modified: trunk/Source/_javascript_Core/profiler/ProfilerBytecode.h (136719 => 136720)
--- trunk/Source/_javascript_Core/profiler/ProfilerBytecode.h 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/profiler/ProfilerBytecode.h 2012-12-05 19:18:52 UTC (rev 136720)
@@ -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 @@
{
}
- 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;
};
Modified: trunk/Source/_javascript_Core/profiler/ProfilerDatabase.cpp (136719 => 136720)
--- trunk/Source/_javascript_Core/profiler/ProfilerDatabase.cpp 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/profiler/ProfilerDatabase.cpp 2012-12-05 19:18:52 UTC (rev 136720)
@@ -80,7 +80,7 @@
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(
Modified: trunk/Source/_javascript_Core/runtime/CommonIdentifiers.h (136719 => 136720)
--- trunk/Source/_javascript_Core/runtime/CommonIdentifiers.h 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Source/_javascript_Core/runtime/CommonIdentifiers.h 2012-12-05 19:18:52 UTC (rev 136720)
@@ -67,6 +67,7 @@
macro(name) \
macro(now) \
macro(Object) \
+ macro(opcode) \
macro(origin) \
macro(parse) \
macro(propertyIsEnumerable) \
Modified: trunk/Tools/ChangeLog (136719 => 136720)
--- trunk/Tools/ChangeLog 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Tools/ChangeLog 2012-12-05 19:18:52 UTC (rev 136720)
@@ -1,3 +1,18 @@
+2012-12-05 Filip Pizlo <[email protected]>
+
+ 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 <[email protected]>
Fix compilation for Qt5.0.0 stable branch.
Modified: trunk/Tools/Scripts/display-profiler-output (136719 => 136720)
--- trunk/Tools/Scripts/display-profiler-output 2012-12-05 19:18:13 UTC (rev 136719)
+++ trunk/Tools/Scripts/display-profiler-output 2012-12-05 19:18:52 UTC (rev 136720)
@@ -29,14 +29,19 @@
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 @@
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 @@
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 @@
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 {
