get_by_id should support caching unset properties in the LLInt
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 May 2016 18:36:30 +0000 (18:36 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 May 2016 18:36:30 +0000 (18:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158136

Reviewed by Benjamin Poulain.

Recently, we started supporting prototype load caching for get_by_id
in the LLInt. This patch extends that to caching unset properties.
While it is uncommon in general for a program to see a single structure
without a given property, the Array.prototype.concat function needs to
lookup the Symbol.isConcatSpreadable property. For any existing code
That property will never be set as it did not exist prior to ES6.

Similarly to the get_by_id_proto_load bytecode, this patch adds a new
bytecode, get_by_id_unset that checks the structureID of the base and
assigns undefined to the result.

There are no new tests here since we already have many tests that
incidentally cover this change.

* bytecode/BytecodeList.json:
* bytecode/BytecodeUseDef.h:
(JSC::computeUsesForBytecodeOffset):
(JSC::computeDefsForBytecodeOffset):
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::printGetByIdOp):
(JSC::CodeBlock::dumpBytecode):
(JSC::CodeBlock::finalizeLLIntInlineCaches):
* bytecode/GetByIdStatus.cpp:
(JSC::GetByIdStatus::computeFromLLInt):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGCapabilities.cpp:
(JSC::DFG::capabilityLevel):
* jit/JIT.cpp:
(JSC::JIT::privateCompileMainPass):
(JSC::JIT::privateCompileSlowCases):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::setupGetByIdPrototypeCache):
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* llint/LLIntSlowPaths.h:
* llint/LowLevelInterpreter32_64.asm:
* llint/LowLevelInterpreter64.asm:

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

12 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/BytecodeList.json
Source/JavaScriptCore/bytecode/BytecodeUseDef.h
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGCapabilities.cpp
Source/JavaScriptCore/jit/JIT.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.h
Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
Source/JavaScriptCore/llint/LowLevelInterpreter64.asm

index 763b276..bc74942 100644 (file)
@@ -1,3 +1,48 @@
+2016-05-27  Keith Miller  <keith_miller@apple.com>
+
+        get_by_id should support caching unset properties in the LLInt
+        https://bugs.webkit.org/show_bug.cgi?id=158136
+
+        Reviewed by Benjamin Poulain.
+
+        Recently, we started supporting prototype load caching for get_by_id
+        in the LLInt. This patch extends that to caching unset properties.
+        While it is uncommon in general for a program to see a single structure
+        without a given property, the Array.prototype.concat function needs to
+        lookup the Symbol.isConcatSpreadable property. For any existing code
+        That property will never be set as it did not exist prior to ES6.
+
+        Similarly to the get_by_id_proto_load bytecode, this patch adds a new
+        bytecode, get_by_id_unset that checks the structureID of the base and
+        assigns undefined to the result.
+
+        There are no new tests here since we already have many tests that
+        incidentally cover this change.
+
+        * bytecode/BytecodeList.json:
+        * bytecode/BytecodeUseDef.h:
+        (JSC::computeUsesForBytecodeOffset):
+        (JSC::computeDefsForBytecodeOffset):
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::printGetByIdOp):
+        (JSC::CodeBlock::dumpBytecode):
+        (JSC::CodeBlock::finalizeLLIntInlineCaches):
+        * bytecode/GetByIdStatus.cpp:
+        (JSC::GetByIdStatus::computeFromLLInt):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * dfg/DFGCapabilities.cpp:
+        (JSC::DFG::capabilityLevel):
+        * jit/JIT.cpp:
+        (JSC::JIT::privateCompileMainPass):
+        (JSC::JIT::privateCompileSlowCases):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::setupGetByIdPrototypeCache):
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        * llint/LLIntSlowPaths.h:
+        * llint/LowLevelInterpreter32_64.asm:
+        * llint/LowLevelInterpreter64.asm:
+
 2016-05-26  Filip Pizlo  <fpizlo@apple.com>
 
         Bogus uses of regexp matching should realize that they will OOM before they start swapping
index 21ffb38..01efcc1 100644 (file)
@@ -61,6 +61,7 @@
             { "name" : "op_get_array_length", "length" : 9 },
             { "name" : "op_get_by_id", "length" : 9  },
             { "name" : "op_get_by_id_proto_load", "length" : 9 },
+            { "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 },
index 6a7f75e..3f8ca2d 100644 (file)
@@ -159,6 +159,7 @@ void computeUsesForBytecodeOffset(
     case op_try_get_by_id:
     case op_get_by_id:
     case op_get_by_id_proto_load:
+    case op_get_by_id_unset:
     case op_get_array_length:
     case op_typeof:
     case op_is_empty:
@@ -394,6 +395,7 @@ void computeDefsForBytecodeOffset(CodeBlock* codeBlock, BytecodeBasicBlock* bloc
     case op_try_get_by_id:
     case op_get_by_id:
     case op_get_by_id_proto_load:
+    case op_get_by_id_unset:
     case op_get_by_id_with_this:
     case op_get_by_val_with_this:
     case op_get_array_length:
index 04e3619..8c7b6e5 100644 (file)
@@ -349,6 +349,9 @@ void CodeBlock::printGetByIdOp(PrintStream& out, ExecState* exec, int location,
     case op_get_by_id_proto_load:
         op = "get_by_id_proto_load";
         break;
+    case op_get_by_id_unset:
+        op = "get_by_id_unset";
+        break;
     case op_get_array_length:
         op = "array_length";
         break;
@@ -1119,6 +1122,7 @@ void CodeBlock::dumpBytecode(
         }
         case op_get_by_id:
         case op_get_by_id_proto_load:
+        case op_get_by_id_unset:
         case op_get_array_length: {
             printGetByIdOp(out, exec, location, it);
             printGetByIdCacheStatus(out, exec, location, stubInfos);
@@ -2806,7 +2810,8 @@ void CodeBlock::finalizeLLIntInlineCaches()
         Instruction* curInstruction = &instructions()[propertyAccessInstructions[i]];
         switch (interpreter->getOpcodeID(curInstruction[0].u.opcode)) {
         case op_get_by_id:
-        case op_get_by_id_proto_load: {
+        case op_get_by_id_proto_load:
+        case op_get_by_id_unset: {
             StructureID oldStructureID = curInstruction[4].u.structureID;
             if (!oldStructureID || Heap::isMarked(m_vm->heap.structureIDTable().get(oldStructureID)))
                 break;
index 59e81c7..bab2cb1 100644 (file)
@@ -78,9 +78,11 @@ GetByIdStatus GetByIdStatus::computeFromLLInt(CodeBlock* profiledBlock, unsigned
 
     Opcode opcode = instruction[0].u.opcode;
 
+    ASSERT(opcode == LLInt::getOpcode(op_get_array_length) || opcode == LLInt::getOpcode(op_try_get_by_id) || opcode == LLInt::getOpcode(op_get_by_id_proto_load) || opcode == LLInt::getOpcode(op_get_by_id) || opcode == LLInt::getOpcode(op_get_by_id_unset));
+
     // FIXME: We should not just bail if we see a try_get_by_id or a get_by_id_proto_load.\ e
     // https://bugs.webkit.org/show_bug.cgi?id=158039
-    if (opcode == LLInt::getOpcode(op_get_array_length) || opcode == LLInt::getOpcode(op_try_get_by_id) || opcode == LLInt::getOpcode(op_get_by_id_proto_load))
+    if (opcode != LLInt::getOpcode(op_get_by_id))
         return GetByIdStatus(NoInformation, false);
 
     StructureID structureID = instruction[4].u.structureID;
index 3c28122..a3e8b8f 100644 (file)
@@ -4082,6 +4082,7 @@ bool ByteCodeParser::parseBlock(unsigned limit)
 
         case op_get_by_id:
         case op_get_by_id_proto_load:
+        case op_get_by_id_unset:
         case op_get_array_length: {
             SpeculatedType prediction = getPrediction();
             
index 062427c..8453560 100644 (file)
@@ -155,6 +155,7 @@ CapabilityLevel capabilityLevel(OpcodeID opcodeID, CodeBlock* codeBlock, Instruc
     case op_try_get_by_id:
     case op_get_by_id:
     case op_get_by_id_proto_load:
+    case op_get_by_id_unset:
     case op_get_by_id_with_this:
     case op_get_by_val_with_this:
     case op_get_array_length:
index 92aac99..bf64bba 100644 (file)
@@ -241,6 +241,7 @@ void JIT::privateCompileMainPass()
         DEFINE_OP(op_try_get_by_id)
         case op_get_array_length:
         case op_get_by_id_proto_load:
+        case op_get_by_id_unset:
         DEFINE_OP(op_get_by_id)
         DEFINE_OP(op_get_by_id_with_this)
         DEFINE_OP(op_get_by_val)
@@ -424,6 +425,7 @@ void JIT::privateCompileSlowCases()
         DEFINE_SLOWCASE_OP(op_try_get_by_id)
         case op_get_array_length:
         case op_get_by_id_proto_load:
+        case op_get_by_id_unset:
         DEFINE_SLOWCASE_OP(op_get_by_id)
         DEFINE_SLOWCASE_OP(op_get_by_val)
         DEFINE_SLOWCASE_OP(op_instanceof)
index a103588..9e588b9 100644 (file)
@@ -589,7 +589,11 @@ static void setupGetByIdPrototypeCache(ExecState* exec, VM& vm, Instruction* pc,
     if (structure->typeInfo().prohibitsPropertyCaching() || structure->isDictionary())
         return;
 
-    ObjectPropertyConditionSet conditions = generateConditionsForPrototypePropertyHit(vm, codeBlock, exec, structure, slot.slotBase(), ident.impl());
+    ObjectPropertyConditionSet conditions;
+    if (slot.isUnset())
+        conditions = generateConditionsForPropertyMiss(vm, codeBlock, exec, structure, ident.impl());
+    else
+        conditions = generateConditionsForPrototypePropertyHit(vm, codeBlock, exec, structure, slot.slotBase(), ident.impl());
 
     if (!conditions.isValid())
         return;
@@ -604,16 +608,22 @@ static void setupGetByIdPrototypeCache(ExecState* exec, VM& vm, Instruction* pc,
             offset = condition.condition().offset();
         result.iterator->value.add(condition, pc)->install();
     }
-    ASSERT(offset != invalidOffset);
+    ASSERT((offset == invalidOffset) == slot.isUnset());
 
     ConcurrentJITLocker locker(codeBlock->m_lock);
 
+    if (slot.isUnset()) {
+        pc[0].u.opcode = LLInt::getOpcode(op_get_by_id_unset);
+        pc[4].u.structureID = structure->id();
+        return;
+    }
+    ASSERT(slot.isValue());
+
     pc[0].u.opcode = LLInt::getOpcode(op_get_by_id_proto_load);
     pc[4].u.structureID = structure->id();
     pc[5].u.operand = offset;
-    // We know that this pointer will remain valid because it is the prototype of some structure, s,
-    // watchpointed above. If any object with structure s were to change prototypes then the conditions
-    // for this cache would fail and this value will never be used again.
+    // We know that this pointer will remain valid because it will be cleared by either a watchpoint fire or
+    // during GC when we clear the LLInt caches.
     pc[6].u.pointer = slot.slotBase();
 }
 
@@ -632,11 +642,11 @@ LLINT_SLOW_PATH_DECL(slow_path_get_by_id)
     
     if (!LLINT_ALWAYS_ACCESS_SLOW
         && baseValue.isCell()
-        && slot.isCacheableValue()) {
+        && slot.isCacheable()) {
 
         JSCell* baseCell = baseValue.asCell();
         Structure* structure = baseCell->structure();
-        if (slot.slotBase() == baseValue) {
+        if (slot.isValue() && slot.slotBase() == baseValue) {
             // Start out by clearing out the old cache.
             pc[0].u.opcode = LLInt::getOpcode(op_get_by_id);
             pc[4].u.pointer = nullptr; // old structure
@@ -653,7 +663,7 @@ LLINT_SLOW_PATH_DECL(slow_path_get_by_id)
                 pc[4].u.structureID = structure->id();
                 pc[5].u.operand = slot.cachedOffset();
             }
-        } else if (UNLIKELY(pc[7].u.operand)) {
+        } else if (UNLIKELY(pc[7].u.operand && (slot.isValue() || slot.isUnset()))) {
             ASSERT(slot.slotBase() != baseValue);
 
             if (!(--pc[7].u.operand))
index da36305..2499542 100644 (file)
@@ -71,7 +71,6 @@ LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_instanceof);
 LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_instanceof_custom);
 LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_try_get_by_id);
 LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_get_by_id);
-LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_get_by_id_proto_load);
 LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_get_arguments_length);
 LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_put_by_id);
 LLINT_SLOW_PATH_HIDDEN_DECL(slow_path_del_by_id);
index 98eea4f..f5f7560 100644 (file)
@@ -1337,8 +1337,9 @@ end
 # opcode for own properties. We also allow for the cache to change anytime it fails,
 # since ping-ponging is free. At best we get lucky and the get_by_id will continue
 # to take fast path on the new cache. At worst we take slow path, which is what
-# we would have been doing anyway. For prototype properties, we will attempt to
-# convert opcode into a get_by_id_proto_load after a execution counter hits zero.
+# we would have been doing anyway. For prototype/unset properties, we will attempt to
+# convert opcode into a get_by_id_proto_load/get_by_id_unset, respectively, after an
+# execution counter hits zero.
 
 _llint_op_get_by_id:
     traceExecution()
@@ -1359,7 +1360,6 @@ _llint_op_get_by_id:
     dispatch(9)
 
 
-
 _llint_op_get_by_id_proto_load:
     traceExecution()
     loadi 8[PC], t0
@@ -1380,6 +1380,23 @@ _llint_op_get_by_id_proto_load:
     dispatch(9)
 
 
+_llint_op_get_by_id_unset:
+    traceExecution()
+    loadi 8[PC], t0
+    loadi 16[PC], t1
+    loadConstantOrVariablePayload(t0, CellTag, t3, .opGetByIdUnsetSlow)
+    bineq JSCell::m_structureID[t3], t1, .opGetByIdUnsetSlow
+    loadi 4[PC], t2
+    storei UndefinedTag, TagOffset[cfr, t2, 8]
+    storei 0, PayloadOffset[cfr, t2, 8]
+    valueProfile(UndefinedTag, 0, 32, t2)
+    dispatch(9)
+
+.opGetByIdUnsetSlow:
+    callSlowPath(_llint_slow_path_get_by_id)
+    dispatch(9)
+
+
 _llint_op_get_array_length:
     traceExecution()
     loadi 8[PC], t0
index 511ba74..b9c17a5 100644 (file)
@@ -1252,6 +1252,23 @@ _llint_op_get_by_id_proto_load:
     dispatch(9)
 
 
+_llint_op_get_by_id_unset:
+    traceExecution()
+    loadisFromInstruction(2, t0)
+    loadConstantOrVariableCell(t0, t3, .opGetByIdUnsetSlow)
+    loadi JSCell::m_structureID[t3], t1
+    loadisFromInstruction(4, t2)
+    bineq t2, t1, .opGetByIdUnsetSlow
+    loadisFromInstruction(1, t2)
+    storeq ValueUndefined, [cfr, t2, 8]
+    valueProfile(ValueUndefined, 8, t1)
+    dispatch(9)
+
+.opGetByIdUnsetSlow:
+    callSlowPath(_llint_slow_path_get_by_id)
+    dispatch(9)
+
+
 _llint_op_get_array_length:
     traceExecution()
     loadisFromInstruction(2, t0)