Changeset 286635 in webkit

Timestamp:

Dec 7, 2021, 5:43:22 PM (5 years ago)

Author:

Ross Kirsling

Message:

[JSC] Add LLInt IC for try_get_by_id of own cacheable value
​https://bugs.webkit.org/show_bug.cgi?id=233830

Reviewed by Yusuke Suzuki.

This patch adds an LLInt IC for the "own cacheable value" path of try_get_by_id;
this is the simplest case and basically the same as get_by_id_direct.

Performance is neutral with JIT enabled as well as on current uses of try_get_by_id in JSC
(e.g. hasObservableSideEffectsForRegexpSplit), but microbenchmarks of try_get_by_id itself see a 2x speedup:

Before After

try-get-by-id-polymorphic 123.8361+-0.4562 ^{61.7586+-0.3770} definitely 2.0052x faster
try-get-by-id-basic 124.4437+-0.6091 ^{61.0340+-0.1924} definitely 2.0389x faster

<geometric> 124.1207+-0.3130 ^{61.3865+-0.2019} definitely 2.0220x faster

• bytecode/BytecodeList.rb:
• bytecode/CodeBlock.cpp:
• bytecode/GetByStatus.cpp:
• llint/LLIntSlowPaths.cpp:
• llint/LowLevelInterpreter.asm:
• llint/LowLevelInterpreter32_64.asm:
• llint/LowLevelInterpreter64.asm:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/BytecodeList.rb (modified) (1 diff)
• bytecode/CodeBlock.cpp (modified) (2 diffs)
• bytecode/GetByStatus.cpp (modified) (1 diff)
• llint/LLIntSlowPaths.cpp (modified) (1 diff)
• llint/LowLevelInterpreter.asm (modified) (1 diff)
• llint/LowLevelInterpreter32_64.asm (modified) (1 diff)
• llint/LowLevelInterpreter64.asm (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-12-07  Ross Kirsling  <ross.kirsling@sony.com>
+
+        [JSC] Add LLInt IC for try_get_by_id of own cacheable value
+        https://bugs.webkit.org/show_bug.cgi?id=233830
+
+        Reviewed by Yusuke Suzuki.
+
+        This patch adds an LLInt IC for the "own cacheable value" path of try_get_by_id;
+        this is the simplest case and basically the same as get_by_id_direct.
+
+        Performance is neutral with JIT enabled as well as on current uses of try_get_by_id in JSC
+        (e.g. hasObservableSideEffectsForRegexpSplit), but microbenchmarks of try_get_by_id itself see a 2x speedup:
+
+                                             Before                    After
+
+        try-get-by-id-polymorphic      123.8361+-0.4562     ^     61.7586+-0.3770        ^ definitely 2.0052x faster
+        try-get-by-id-basic            124.4437+-0.6091     ^     61.0340+-0.1924        ^ definitely 2.0389x faster
+
+        <geometric>                    124.1207+-0.3130     ^     61.3865+-0.2019        ^ definitely 2.0220x faster
+
+
+        * bytecode/BytecodeList.rb:
+        * bytecode/CodeBlock.cpp:
+        * bytecode/GetByStatus.cpp:
+        * llint/LLIntSlowPaths.cpp:
+        * llint/LowLevelInterpreter.asm:
+        * llint/LowLevelInterpreter32_64.asm:
+        * llint/LowLevelInterpreter64.asm:
+
 2021-12-07  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/JavaScriptCore/bytecode/BytecodeList.rb

@@ -524 +524 @@
     metadata: {
         profile: ValueProfile,
+        structureID: StructureID,
+        offset: unsigned,
     }
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -1387 +1387 @@
         });
 
+        m_metadata->forEach<OpTryGetById>([&] (auto& metadata) {
+            StructureID oldStructureID = metadata.m_structureID;
+            if (!oldStructureID || vm.heap.isMarked(oldStructureID.decode()))
+                return;
+            dataLogLnIf(Options::verboseOSR(), "Clearing try_get_by_id LLInt property access.");
+            metadata.m_structureID = StructureID();
+            metadata.m_offset = 0;
+        });
+
         m_metadata->forEach<OpGetByIdDirect>([&] (auto& metadata) {
             StructureID oldStructureID = metadata.m_structureID;
             if (!oldStructureID || vm.heap.isMarked(oldStructureID.decode()))
                 return;
-            dataLogLnIf(Options::verboseOSR(), "Clearing LLInt property access.");
+            dataLogLnIf(Options::verboseOSR(), "Clearing get_by_id_direct LLInt property access.");
             metadata.m_structureID = StructureID();
             metadata.m_offset = 0;
@@ -1462 +1471 @@
                 return;
 
-            dataLogLnIf(Options::verboseOSR(), "Clearing LLInt set_private_brand transition.");
+            dataLogLnIf(Options::verboseOSR(), "Clearing LLInt check_private_brand transition.");
             metadata.m_structureID = StructureID();
             metadata.m_brand.clear();

trunk/Source/JavaScriptCore/bytecode/GetByStatus.cpp

@@ -71 +71 @@
         break;
     }
+
+    case op_try_get_by_id:
+        structureID = instruction->as<OpTryGetById>().metadata(profiledBlock).m_structureID;
+        identifier = &(profiledBlock->identifier(instruction->as<OpTryGetById>().m_property));
+        break;
     case op_get_by_id_direct:
         structureID = instruction->as<OpGetByIdDirect>().metadata(profiledBlock).m_structureID;
         identifier = &(profiledBlock->identifier(instruction->as<OpGetByIdDirect>().m_property));
         break;
-    case op_try_get_by_id: {
-        // FIXME: We should not just bail if we see a try_get_by_id.
-        // https://bugs.webkit.org/show_bug.cgi?id=158039
-        return GetByStatus(NoInformation, false);
-    }
 
     case op_get_by_val:

trunk/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp

@@ -665 +665 @@
     JSValue result = slot.getPureResult();
 
+    if (!LLINT_ALWAYS_ACCESS_SLOW && slot.isCacheable() && !slot.isUnset()) {
+        ASSERT(!slot.isTaintedByOpaqueObject());
+        ASSERT(baseValue.isCell());
+
+        auto& metadata = bytecode.metadata(codeBlock);
+        {
+            StructureID oldStructureID = metadata.m_structureID;
+            if (oldStructureID) {
+                Structure* a = oldStructureID.decode();
+                Structure* b = baseValue.asCell()->structure(vm);
+
+                if (Structure::shouldConvertToPolyProto(a, b)) {
+                    ASSERT(a->rareData()->sharedPolyProtoWatchpoint().get() == b->rareData()->sharedPolyProtoWatchpoint().get());
+                    a->rareData()->sharedPolyProtoWatchpoint()->invalidate(vm, StringFireDetail("Detected poly proto opportunity."));
+                }
+            }
+        }
+
+        JSCell* baseCell = baseValue.asCell();
+        Structure* structure = baseCell->structure(vm);
+        if (slot.isValue() && slot.slotBase() == baseValue) {
+            // Start out by clearing out the old cache.
+            metadata.m_structureID = StructureID();
+            metadata.m_offset = 0;
+
+            if (structure->propertyAccessesAreCacheable() && !structure->needImpurePropertyWatchpoint()) {
+                {
+                    ConcurrentJSLocker locker(codeBlock->m_lock);
+                    metadata.m_structureID = structure->id();
+                    metadata.m_offset = slot.cachedOffset();
+                }
+                vm.writeBarrier(codeBlock);
+            }
+        }
+    }
+
     LLINT_RETURN_PROFILED(result);
 }

trunk/Source/JavaScriptCore/llint/LowLevelInterpreter.asm

@@ -2181 +2181 @@
 llintSlowPathOp(super_sampler_end)
 llintSlowPathOp(throw)
-llintSlowPathOp(try_get_by_id)
 llintSlowPathOp(get_by_id_with_this)
 

trunk/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm

@@ -1422 +1422 @@
 # execution counter hits zero.
 
+llintOpWithMetadata(op_try_get_by_id, OpTryGetById, macro (size, get, dispatch, metadata, return)
+    metadata(t5, t0)
+    get(m_base, t0)
+    loadi OpTryGetById::Metadata::m_structureID[t5], t1
+    loadConstantOrVariablePayload(size, t0, CellTag, t3, .opTryGetByIdSlow)
+    loadi OpTryGetById::Metadata::m_offset[t5], t2
+    bineq JSCell::m_structureID[t3], t1, .opTryGetByIdSlow
+    loadPropertyAtVariableOffset(t2, t3, t0, t1)
+    valueProfile(OpTryGetById, m_profile, t5, t0, t1)
+    return(t0, t1)
+
+.opTryGetByIdSlow:
+    callSlowPath(_llint_slow_path_try_get_by_id)
+    dispatch()
+end)
+
 llintOpWithMetadata(op_get_by_id_direct, OpGetByIdDirect, macro (size, get, dispatch, metadata, return)
     metadata(t5, t0)

trunk/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm

@@ -1533 +1533 @@
 end
 
+
+llintOpWithMetadata(op_try_get_by_id, OpTryGetById, macro (size, get, dispatch, metadata, return)
+    metadata(t2, t0)
+    get(m_base, t0)
+    loadConstantOrVariableCell(size, t0, t3, .opTryGetByIdSlow)
+    loadi JSCell::m_structureID[t3], t1
+    loadi OpTryGetById::Metadata::m_structureID[t2], t0
+    bineq t0, t1, .opTryGetByIdSlow
+    loadi OpTryGetById::Metadata::m_offset[t2], t1
+    loadPropertyAtVariableOffset(t1, t3, t0)
+    valueProfile(OpTryGetById, m_profile, t2, t0)
+    return(t0)
+
+.opTryGetByIdSlow:
+    callSlowPath(_llint_slow_path_try_get_by_id)
+    dispatch()
+end)
 
 llintOpWithMetadata(op_get_by_id_direct, OpGetByIdDirect, macro (size, get, dispatch, metadata, return)