TryGetById should have a ValueProfile so that it can predict its output type
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Aug 2016 22:11:59 +0000 (22:11 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Aug 2016 22:11:59 +0000 (22:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=160921

Patch by JF Bastien <jfbastien@apple.com> on 2016-08-25
Reviewed by Saam Barati.

JSTests:

* microbenchmarks/try-get-by-id-basic.js: Added.
(const.check):
(const.bench.f.const.fooPlusBar.createBuiltin):
* microbenchmarks/try-get-by-id-polymorphic.js: Added.
(const.check):
(fooPlusBar.createBuiltin):
(bench):

Source/JavaScriptCore:

Add a ValueProfile to TryGetById, and make sure DFG picks it up.

A microbenchmark for perfectly predicted computation shows a 20%
runtime reduction with no hit if the prediction goes polymorphic.

* bytecode/BytecodeList.json:
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::dumpBytecode):
(JSC::CodeBlock::finishCreation):
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::emitTryGetById):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGNode.h:
(JSC::DFG::Node::hasHeapPrediction):
* dfg/DFGPredictionPropagationPhase.cpp:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* jit/JITPropertyAccess.cpp:
(JSC::JIT::emit_op_try_get_by_id):
* jit/JITPropertyAccess32_64.cpp:
(JSC::JIT::emit_op_try_get_by_id):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* llint/LowLevelInterpreter.asm:

Source/WTF:

Add WTF_CONCAT to StdLibExtras.h

* wtf/StdLibExtras.h:

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

18 files changed:
JSTests/ChangeLog
JSTests/microbenchmarks/try-get-by-id-basic.js [new file with mode: 0644]
JSTests/microbenchmarks/try-get-by-id-polymorphic.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/BytecodeList.json
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/jit/JITPropertyAccess.cpp
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/JavaScriptCore/llint/LowLevelInterpreter.asm
Source/WTF/ChangeLog
Source/WTF/wtf/StdLibExtras.h

index 2792e61..379d544 100644 (file)
@@ -1,3 +1,18 @@
+2016-08-25  JF Bastien  <jfbastien@apple.com>
+
+        TryGetById should have a ValueProfile so that it can predict its output type
+        https://bugs.webkit.org/show_bug.cgi?id=160921
+
+        Reviewed by Saam Barati.
+
+        * microbenchmarks/try-get-by-id-basic.js: Added.
+        (const.check):
+        (const.bench.f.const.fooPlusBar.createBuiltin):
+        * microbenchmarks/try-get-by-id-polymorphic.js: Added.
+        (const.check):
+        (fooPlusBar.createBuiltin):
+        (bench):
+
 2016-08-25  Caio Lima  <ticaiolima@gmail.com>
 
         NewRegexp should not prevent inlining
diff --git a/JSTests/microbenchmarks/try-get-by-id-basic.js b/JSTests/microbenchmarks/try-get-by-id-basic.js
new file mode 100644 (file)
index 0000000..1362457
--- /dev/null
@@ -0,0 +1,28 @@
+// Test tryGetById's value profiling feedback without going too polymorphic.
+
+var it = 1e5;
+
+const check = (got, expect) => { if (got != expect) throw "Error: bad result got " + got + " expected " + expect; };
+
+const bench = f => {
+    // Re-create the builtin each time, so each benchmark gets its own value prediction.
+    const fooPlusBar = createBuiltin(`(function(o) { return @tryGetById(o, "foo") + @tryGetById(o, "bar"); })`);
+    noInline(fooPlusBar);
+    f(fooPlusBar);
+}
+noInline(bench);
+
+// Non bool int32.
+o = { foo: 42, bar: 1337 };
+bench(builtin => { var res = 0; for (var i = 0; i < it; ++i) res += builtin(o);  check(res, (o.foo + o.bar) * it); });
+
+// Non int double.
+p = { foo: Math.PI, bar: Math.E };
+bench(builtin => { var res = 0.; for (var i = 0; i < it; ++i) res += builtin(p); check(Math.round(res), Math.round((p.foo + p.bar) * it)); });
+
+// String ident.
+s = { foo: "", bar: "⌽" };
+bench(builtin => { var res = ""; for (var i = 0; i < it; ++i) res += builtin(s); check(res.length, (s.foo.length + s.bar.length) * it); });
+
+// Again: non bool int32.
+bench(builtin => { var res = 0; for (var i = 0; i < it; ++i) res += builtin(o); check(res, (o.foo + o.bar) * it); });
diff --git a/JSTests/microbenchmarks/try-get-by-id-polymorphic.js b/JSTests/microbenchmarks/try-get-by-id-polymorphic.js
new file mode 100644 (file)
index 0000000..7624984
--- /dev/null
@@ -0,0 +1,27 @@
+// Test tryGetById's value profiling feedback after it's too polymorphic.
+
+var it = 1e5;
+
+const check = (got, expect) => { if (got != expect) throw "Error: bad result got " + got + " expected " + expect; };
+
+fooPlusBar = createBuiltin(`(function(o) { return @tryGetById(o, "foo") + @tryGetById(o, "bar"); })`);
+noInline(fooPlusBar);
+
+const bench = f => { f(); }
+noInline(bench);
+
+// Non bool int32.
+o = { foo: 42, bar: 1337 };
+bench(() => { var res = 0; for (var i = 0; i < it; ++i) res += fooPlusBar(o);  check(res, (o.foo + o.bar) * it); });
+
+// Non int double.
+p = { foo: Math.PI, bar: Math.E };
+bench(() => { var res = 0.; for (var i = 0; i < it; ++i) res += fooPlusBar(p); check(Math.round(res), Math.round((p.foo + p.bar) * it)); });
+
+// String ident.
+// This gets too polymorphic for the engine's taste.
+s = { foo: "", bar: "⌽" };
+bench(() => { var res = ""; for (var i = 0; i < it; ++i) res += fooPlusBar(s); check(res.length, (s.foo.length + s.bar.length) * it); });
+
+// Again: non bool int32.
+bench(() => { var res = 0; for (var i = 0; i < it; ++i) res += fooPlusBar(o); check(res, (o.foo + o.bar) * it); });
index c821c14..2468bf7 100644 (file)
@@ -1,3 +1,38 @@
+2016-08-25  JF Bastien  <jfbastien@apple.com>
+
+        TryGetById should have a ValueProfile so that it can predict its output type
+        https://bugs.webkit.org/show_bug.cgi?id=160921
+
+        Reviewed by Saam Barati.
+
+        Add a ValueProfile to TryGetById, and make sure DFG picks it up.
+
+        A microbenchmark for perfectly predicted computation shows a 20%
+        runtime reduction with no hit if the prediction goes polymorphic.
+
+        * bytecode/BytecodeList.json:
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::dumpBytecode):
+        (JSC::CodeBlock::finishCreation):
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitTryGetById):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::hasHeapPrediction):
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * jit/JITPropertyAccess.cpp:
+        (JSC::JIT::emit_op_try_get_by_id):
+        * jit/JITPropertyAccess32_64.cpp:
+        (JSC::JIT::emit_op_try_get_by_id):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        * llint/LowLevelInterpreter.asm:
+
 2016-08-25  Csaba Osztrogonác  <ossy@webkit.org>
 
         generate-js-builtins.py should generate platform independent files
index e38025d..43ecf87 100644 (file)
@@ -67,7 +67,7 @@
             { "name" : "op_get_by_id_unset", "length" : 9 },
             { "name" : "op_get_by_id_with_this", "length" : 5 },
             { "name" : "op_get_by_val_with_this", "length" : 5 },
-            { "name" : "op_try_get_by_id", "length" : 4 },
+            { "name" : "op_try_get_by_id", "length" : 5 },
             { "name" : "op_put_by_id", "length" : 9 },
             { "name" : "op_put_by_id_with_this", "length" : 5 },
             { "name" : "op_del_by_id", "length" : 4 },
index 1d9135a..d61b885 100644 (file)
@@ -1141,6 +1141,7 @@ void CodeBlock::dumpBytecode(
             int id0 = (++it)->u.operand;
             printLocationAndOp(out, exec, location, it, "try_get_by_id");
             out.printf("%s, %s, %s", registerName(r0).data(), registerName(r1).data(), idName(id0, identifier(id0)).data());
+            dumpValueProfiling(out, it, hasPrintedProfiling);
             break;
         }
         case op_get_by_id:
@@ -2106,6 +2107,7 @@ void CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
         }
         case op_get_direct_pname:
         case op_get_by_id:
+        case op_try_get_by_id:
         case op_get_from_arguments:
         case op_to_number: {
             ValueProfile* profile = &m_valueProfiles[pc[opLength - 1].u.operand];
index 312d89f..b6df15b 100644 (file)
@@ -2454,10 +2454,11 @@ RegisterID* BytecodeGenerator::emitTryGetById(RegisterID* dst, RegisterID* base,
 {
     ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties are not supported with tryGetById.");
 
-    emitOpcode(op_try_get_by_id);
+    UnlinkedValueProfile profile = emitProfiledOpcode(op_try_get_by_id);
     instructions().append(kill(dst));
     instructions().append(base->index());
     instructions().append(addConstant(property));
+    instructions().append(profile);
     return dst;
 }
 
index 8492b52..55549ff 100644 (file)
@@ -238,7 +238,6 @@ private:
 
     Node* store(Node* base, unsigned identifier, const PutByIdVariant&, Node* value);
 
-    void handleTryGetById(int destinationOperand, Node* base, unsigned identifierNumber, const GetByIdStatus&);
     void handleGetById(
         int destinationOperand, SpeculatedType, Node* base, unsigned identifierNumber, GetByIdStatus, AccessType);
     void emitPutById(
@@ -1204,14 +1203,26 @@ private:
     bool m_hasDebuggerEnabled;
 };
 
+// The idiom:
+//     if (true) { ...; goto label; } else label: continue
+// Allows using NEXT_OPCODE as a statement, even in unbraced if+else, while containing a `continue`.
+// The more common idiom:
+//     do { ...; } while (false)
+// Doesn't allow using `continue`.
 #define NEXT_OPCODE(name) \
-    m_currentIndex += OPCODE_LENGTH(name); \
-    continue
-
+    if (true) { \
+        m_currentIndex += OPCODE_LENGTH(name); \
+        goto WTF_CONCAT(NEXT_OPCODE_, __LINE__); /* Need a unique label: usable more than once per function. */ \
+    } else \
+    WTF_CONCAT(NEXT_OPCODE_, __LINE__): \
+        continue
+
+// Chain expressions with comma-operator so LAST_OPCODE can be used as a statement.
 #define LAST_OPCODE(name) \
-    m_currentIndex += OPCODE_LENGTH(name); \
-    m_exitOK = false; \
-    return shouldContinueParsing
+    return \
+        m_currentIndex += OPCODE_LENGTH(name), \
+        m_exitOK = false, \
+        shouldContinueParsing
 
 ByteCodeParser::Terminality ByteCodeParser::handleCall(Instruction* pc, NodeType op, CallMode callMode)
 {
@@ -4189,21 +4200,7 @@ bool ByteCodeParser::parseBlock(unsigned limit)
             NEXT_OPCODE(op_put_by_val_with_this);
         }
 
-        case op_try_get_by_id: {
-            Node* base = get(VirtualRegister(currentInstruction[2].u.operand));
-            unsigned identifierNumber = m_inlineStackTop->m_identifierRemap[currentInstruction[3].u.operand];
-            UniquedStringImpl* uid = m_graph.identifiers()[identifierNumber];
-
-            GetByIdStatus getByIdStatus = GetByIdStatus::computeFor(
-                m_inlineStackTop->m_profiledBlock, m_dfgCodeBlock,
-                m_inlineStackTop->m_stubInfos, m_dfgStubInfos,
-                currentCodeOrigin(), uid);
-
-            handleGetById(currentInstruction[1].u.operand, SpecHeapTop, base, identifierNumber, getByIdStatus, AccessType::GetPure);
-
-            NEXT_OPCODE(op_try_get_by_id);
-        }
-
+        case op_try_get_by_id:
         case op_get_by_id:
         case op_get_by_id_proto_load:
         case op_get_by_id_unset:
@@ -4218,11 +4215,15 @@ bool ByteCodeParser::parseBlock(unsigned limit)
                 m_inlineStackTop->m_profiledBlock, m_dfgCodeBlock,
                 m_inlineStackTop->m_stubInfos, m_dfgStubInfos,
                 currentCodeOrigin(), uid);
+            AccessType type = op_try_get_by_id == opcodeID ? AccessType::GetPure : AccessType::Get;
             
             handleGetById(
-                currentInstruction[1].u.operand, prediction, base, identifierNumber, getByIdStatus, AccessType::Get);
+                currentInstruction[1].u.operand, prediction, base, identifierNumber, getByIdStatus, type);
 
-            NEXT_OPCODE(op_get_by_id);
+            if (op_try_get_by_id == opcodeID)
+                NEXT_OPCODE(op_try_get_by_id); // Opcode's length is different from others in this case.
+            else
+                NEXT_OPCODE(op_get_by_id);
         }
         case op_get_by_id_with_this: {
             Node* base = get(VirtualRegister(currentInstruction[2].u.operand));
@@ -4557,11 +4558,10 @@ bool ByteCodeParser::parseBlock(unsigned limit)
             // If the call is terminal then we should not parse any further bytecodes as the TailCall will exit the function.
             // If the call is not terminal, however, then we want the subsequent op_ret/op_jump to update metadata and clean
             // things up.
-            if (terminality == NonTerminal) {
+            if (terminality == NonTerminal)
                 NEXT_OPCODE(op_tail_call);
-            } else {
+            else
                 LAST_OPCODE(op_tail_call);
-            }
         }
 
         case op_construct:
@@ -4582,11 +4582,10 @@ bool ByteCodeParser::parseBlock(unsigned limit)
             // If the call is terminal then we should not parse any further bytecodes as the TailCall will exit the function.
             // If the call is not terminal, however, then we want the subsequent op_ret/op_jump to update metadata and clean
             // things up.
-            if (terminality == NonTerminal) {
+            if (terminality == NonTerminal)
                 NEXT_OPCODE(op_tail_call_varargs);
-            } else {
+            else
                 LAST_OPCODE(op_tail_call_varargs);
-            }
         }
 
         case op_tail_call_forward_arguments: {
@@ -4599,11 +4598,10 @@ bool ByteCodeParser::parseBlock(unsigned limit)
             // If the call is terminal then we should not parse any further bytecodes as the TailCall will exit the function.
             // If the call is not terminal, however, then we want the subsequent op_ret/op_jump to update metadata and clean
             // things up.
-            if (terminality == NonTerminal) {
+            if (terminality == NonTerminal)
                 NEXT_OPCODE(op_tail_call);
-            } else {
+            else
                 LAST_OPCODE(op_tail_call);
-            }
         }
             
         case op_construct_varargs: {
index 75455ec..6171e2f 100644 (file)
@@ -1404,6 +1404,7 @@ public:
         case GetDirectPname:
         case GetById:
         case GetByIdFlush:
+        case TryGetById:
         case GetByVal:
         case Call:
         case TailCallInlinedCaller:
index 49609ba..48c7f85 100644 (file)
@@ -685,10 +685,6 @@ private:
             setPrediction(SpecBytecodeTop);
             break;
         }
-        case TryGetById: {
-            setPrediction(SpecBytecodeTop);
-            break;
-        }
         case ArrayPop:
         case ArrayPush:
         case RegExpExec:
@@ -697,6 +693,7 @@ private:
         case StringReplaceRegExp:
         case GetById:
         case GetByIdFlush:
+        case TryGetById:
         case GetByOffset:
         case MultiGetByOffset:
         case GetDirectPname:
index 8af7610..c698b96 100644 (file)
@@ -4153,6 +4153,8 @@ void SpeculativeJIT::compile(Node* node)
     }
 
     case GetById: {
+        // FIXME https://bugs.webkit.org/show_bug.cgi?id=161158
+        // dedup with SpeculativeJIT::compileTryGetById and 64-bit version of this.
         switch (node->child1().useKind()) {
         case CellUse: {
             SpeculateCellOperand base(this, node->child1());
index 64ed2dc..4d2c638 100644 (file)
@@ -4095,6 +4095,8 @@ void SpeculativeJIT::compile(Node* node)
     }
 
     case GetById: {
+        // FIXME https://bugs.webkit.org/show_bug.cgi?id=161158
+        // dedup with SpeculativeJIT::compileTryGetById and 32-bit version of this.
         switch (node->child1().useKind()) {
         case CellUse: {
             SpeculateCellOperand base(this, node->child1());
index bba0f01..3343642 100644 (file)
@@ -584,6 +584,7 @@ void JIT::emit_op_try_get_by_id(Instruction* currentInstruction)
     addSlowCase(gen.slowPathJump());
     m_getByIds.append(gen);
     
+    emitValueProfilingSite();
     emitPutVirtualRegister(resultVReg);
 }
 
index 2ce50a2..32a8648 100644 (file)
@@ -599,6 +599,7 @@ void JIT::emit_op_try_get_by_id(Instruction* currentInstruction)
     addSlowCase(gen.slowPathJump());
     m_getByIds.append(gen);
     
+    emitValueProfilingSite();
     emitStore(dst, regT1, regT0);
 }
 
index 452b025..4b99a4a 100644 (file)
@@ -572,8 +572,9 @@ LLINT_SLOW_PATH_DECL(slow_path_try_get_by_id)
     PropertySlot slot(baseValue, PropertySlot::PropertySlot::InternalMethodType::VMInquiry);
 
     baseValue.getPropertySlot(exec, ident, slot);
+    JSValue result = slot.getPureResult();
 
-    LLINT_RETURN(slot.getPureResult());
+    LLINT_RETURN_PROFILED(op_try_get_by_id, result);
 }
 
 static void setupGetByIdPrototypeCache(ExecState* exec, VM& vm, Instruction* pc, JSCell* baseCell, PropertySlot& slot, const Identifier& ident)
index 14918b6..04af7fa 100644 (file)
@@ -1405,7 +1405,7 @@ _llint_op_in:
 _llint_op_try_get_by_id:
     traceExecution()
     callOpcodeSlowPath(_llint_slow_path_try_get_by_id)
-    dispatch(4)
+    dispatch(5)
 
 
 _llint_op_del_by_id:
index 0a84192..331aaeb 100644 (file)
@@ -1,3 +1,14 @@
+2016-08-25  JF Bastien  <jfbastien@apple.com>
+
+        TryGetById should have a ValueProfile so that it can predict its output type
+        https://bugs.webkit.org/show_bug.cgi?id=160921
+
+        Reviewed by Saam Barati.
+
+        Add WTF_CONCAT to StdLibExtras.h
+
+        * wtf/StdLibExtras.h:
+
 2016-08-25  Johan K. Jensen  <johan_jensen@apple.com>
 
         Don't store networkLoadTiming in the disk cache
index 857ec44..01cb532 100644 (file)
 #define STRINGIZE(exp) #exp
 #define STRINGIZE_VALUE_OF(exp) STRINGIZE(exp)
 
+// WTF_CONCAT: concatenate two symbols into one, even expandable macros
+#define WTF_CONCAT_INTERNAL_DONT_USE(a, b) a ## b
+#define WTF_CONCAT(a, b) WTF_CONCAT_INTERNAL_DONT_USE(a, b)
+
+
 // Make "PRId64" format specifier work for Visual C++ on Windows.
 #if OS(WINDOWS) && !defined(PRId64)
 #define PRId64 "lld"