Reland "Refactor OSROptimizedCodeCache"

Tweak a few names, remove a few GetIsolate calls, other minor
usability refactors.

It may be worth taking a closer look at the impl in the future,
currently the design choices don't seem ideal (see the added TODO
on top of the class).

The reland is unchanged from the original CL.

Bug: v8:12161
Change-Id: I9971f7f2fb08b7a1ec2d57b2a0e4accdc11191ca
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3568444Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Jakob Linke <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79759}

src/codegen/compiler.cc

@@ -903,9 +903,8 @@ class OptimizedCodeCache : public AllStatic {
     CodeT code;
     if (IsOSR(osr_offset)) {
       // For OSR, check the OSR optimized code cache.
-      code = function->native_context()
-                 .GetOSROptimizedCodeCache()
-                 .GetOptimizedCode(shared, osr_offset, isolate);
+      code = function->native_context().osr_code_cache().TryGet(
+          shared, osr_offset, isolate);
     } else {
       // Non-OSR code may be cached on the feedback vector.
       if (function->has_feedback_vector()) {
@@ -943,8 +942,8 @@ class OptimizedCodeCache : public AllStatic {
       DCHECK(CodeKindCanOSR(kind));
       Handle<SharedFunctionInfo> shared(function->shared(), isolate);
       Handle<NativeContext> native_context(function->native_context(), isolate);
-      OSROptimizedCodeCache::AddOptimizedCode(native_context, shared, code,
-                                              osr_offset);
+      OSROptimizedCodeCache::Insert(isolate, native_context, shared, code,
+                                    osr_offset);
       return;
     }
 

src/deoptimizer/deoptimizer.cc

@@ -377,8 +377,7 @@ void Deoptimizer::DeoptimizeMarkedCodeForContext(NativeContext native_context) {
     isolate->heap()->InvalidateCodeDeoptimizationData(code);
   }
 
-  native_context.GetOSROptimizedCodeCache().EvictMarkedCode(
-      native_context.GetIsolate());
+  native_context.osr_code_cache().EvictDeoptimizedCode(isolate);
 }
 
 void Deoptimizer::DeoptimizeAll(Isolate* isolate) {
@@ -393,7 +392,7 @@ void Deoptimizer::DeoptimizeAll(Isolate* isolate) {
   while (!context.IsUndefined(isolate)) {
     NativeContext native_context = NativeContext::cast(context);
     MarkAllCodeForContext(native_context);
-    OSROptimizedCodeCache::Clear(native_context);
+    OSROptimizedCodeCache::Clear(isolate, native_context);
     DeoptimizeMarkedCodeForContext(native_context);
     context = native_context.next_context_link();
   }
@@ -449,7 +448,7 @@ void Deoptimizer::DeoptimizeFunction(JSFunction function, Code code) {
     // pointers. Update DeoptimizeMarkedCodeForContext to use handles and remove
     // this call from here.
     OSROptimizedCodeCache::Compact(
-        Handle<NativeContext>(function.native_context(), isolate));
+        isolate, Handle<NativeContext>(function.native_context(), isolate));
   }
 }
 

src/execution/tiering-manager.cc

@@ -156,8 +156,7 @@ bool HaveCachedOSRCodeForCurrentBytecodeOffset(UnoptimizedFrame* frame,
   BytecodeArray bytecode = frame->GetBytecodeArray();
   const int bytecode_offset = frame->GetBytecodeOffset();
   if (V8_UNLIKELY(function.shared().osr_code_cache_state() != kNotCached)) {
-    OSROptimizedCodeCache cache =
-        function.native_context().GetOSROptimizedCodeCache();
+    OSROptimizedCodeCache cache = function.native_context().osr_code_cache();
     interpreter::BytecodeArrayIterator iterator(
         handle(bytecode, frame->isolate()));
     for (int jump_offset : cache.GetBytecodeOffsetsFromSFI(function.shared())) {

src/heap/factory.cc

@@ -1184,7 +1184,7 @@ Handle<NativeContext> Factory::NewNativeContext() {
   context.set_math_random_index(Smi::zero());
   context.set_serialized_objects(*empty_fixed_array());
   context.set_microtask_queue(isolate(), nullptr);
-  context.set_osr_code_cache(*empty_weak_fixed_array());
+  context.set_osr_code_cache(*OSROptimizedCodeCache::Empty(isolate()));
   context.set_retained_maps(*empty_weak_array_list());
   return handle(context, isolate());
 }

src/objects/contexts-inl.h

@@ -296,10 +296,6 @@ ScriptContextTable NativeContext::synchronized_script_context_table() const {
       get(SCRIPT_CONTEXT_TABLE_INDEX, kAcquireLoad));
 }
 
-OSROptimizedCodeCache NativeContext::GetOSROptimizedCodeCache() {
-  return OSROptimizedCodeCache::cast(osr_code_cache());
-}
-
 void NativeContext::SetOptimizedCodeListHead(Object head) {
   set(OPTIMIZED_CODE_LIST, head, UPDATE_WEAK_WRITE_BARRIER, kReleaseStore);
 }

src/objects/contexts.h

@@ -369,7 +369,7 @@ enum ContextLookupFlags {
   V(WEAKSET_ADD_INDEX, JSFunction, weakset_add)                                \
   V(WRAPPED_FUNCTION_MAP_INDEX, Map, wrapped_function_map)                     \
   V(RETAINED_MAPS, Object, retained_maps)                                      \
-  V(OSR_CODE_CACHE_INDEX, WeakFixedArray, osr_code_cache)
+  V(OSR_CODE_CACHE_INDEX, OSROptimizedCodeCache, osr_code_cache)
 
 #include "torque-generated/src/objects/contexts-tq.inc"
 
@@ -777,8 +777,6 @@ class NativeContext : public Context {
   inline void SetDeoptimizedCodeListHead(Object head);
   inline Object DeoptimizedCodeListHead();
 
-  inline OSROptimizedCodeCache GetOSROptimizedCodeCache();
-
   void ResetErrorsThrown();
   void IncrementErrorsThrown();
   int GetErrorsThrown();

src/objects/osr-optimized-code-cache.cc

@@ -12,22 +12,28 @@
 namespace v8 {
 namespace internal {
 
-const int OSROptimizedCodeCache::kInitialLength;
-const int OSROptimizedCodeCache::kMaxLength;
+// static
+Handle<OSROptimizedCodeCache> OSROptimizedCodeCache::Empty(Isolate* isolate) {
+  return Handle<OSROptimizedCodeCache>::cast(
+      isolate->factory()->empty_weak_fixed_array());
+}
 
-void OSROptimizedCodeCache::AddOptimizedCode(
-    Handle<NativeContext> native_context, Handle<SharedFunctionInfo> shared,
-    Handle<CodeT> code, BytecodeOffset osr_offset) {
+// static
+void OSROptimizedCodeCache::Insert(Isolate* isolate,
+                                   Handle<NativeContext> native_context,
+                                   Handle<SharedFunctionInfo> shared,
+                                   Handle<CodeT> code,
+                                   BytecodeOffset osr_offset) {
   DCHECK(!osr_offset.IsNone());
-  DCHECK(CodeKindIsOptimizedJSFunction(code->kind()));
-  STATIC_ASSERT(kEntryLength == 3);
-  Isolate* isolate = native_context->GetIsolate();
   DCHECK(!isolate->serializer_enabled());
+  DCHECK(CodeKindIsOptimizedJSFunction(code->kind()));
 
-  Handle<OSROptimizedCodeCache> osr_cache(
-      native_context->GetOSROptimizedCodeCache(), isolate);
+  Handle<OSROptimizedCodeCache> osr_cache(native_context->osr_code_cache(),
+                                          isolate);
 
   DCHECK_EQ(osr_cache->FindEntry(*shared, osr_offset), -1);
+
+  STATIC_ASSERT(kEntryLength == 3);
   int entry = -1;
   for (int index = 0; index < osr_cache->length(); index += kEntryLength) {
     if (osr_cache->Get(index + kSharedOffset)->IsCleared() ||
@@ -37,28 +43,31 @@ void OSROptimizedCodeCache::AddOptimizedCode(
     }
   }
 
-  if (entry == -1 && osr_cache->length() + kEntryLength <= kMaxLength) {
-    entry = GrowOSRCache(native_context, &osr_cache);
-  } else if (entry == -1) {
-    // We reached max capacity and cannot grow further. Reuse an existing entry.
-    // TODO(mythria): We could use better mechanisms (like lru) to replace
-    // existing entries. Though we don't expect this to be a common case, so
-    // for now choosing to replace the first entry.
-    entry = 0;
+  if (entry == -1) {
+    if (osr_cache->length() + kEntryLength <= kMaxLength) {
+      entry = GrowOSRCache(isolate, native_context, &osr_cache);
+    } else {
+      // We reached max capacity and cannot grow further. Reuse an existing
+      // entry.
+      // TODO(mythria): We could use better mechanisms (like lru) to replace
+      // existing entries. Though we don't expect this to be a common case, so
+      // for now choosing to replace the first entry.
+      entry = 0;
+    }
   }
 
   osr_cache->InitializeEntry(entry, *shared, *code, osr_offset);
 }
 
-void OSROptimizedCodeCache::Clear(NativeContext native_context) {
-  native_context.set_osr_code_cache(
-      *native_context.GetIsolate()->factory()->empty_weak_fixed_array());
+void OSROptimizedCodeCache::Clear(Isolate* isolate,
+                                  NativeContext native_context) {
+  native_context.set_osr_code_cache(*OSROptimizedCodeCache::Empty(isolate));
 }
 
-void OSROptimizedCodeCache::Compact(Handle<NativeContext> native_context) {
-  Handle<OSROptimizedCodeCache> osr_cache(
-      native_context->GetOSROptimizedCodeCache(), native_context->GetIsolate());
-  Isolate* isolate = native_context->GetIsolate();
+void OSROptimizedCodeCache::Compact(Isolate* isolate,
+                                    Handle<NativeContext> native_context) {
+  Handle<OSROptimizedCodeCache> osr_cache(native_context->osr_code_cache(),
+                                          isolate);
 
   // Re-adjust the cache so all the valid entries are on one side. This will
   // enable us to compress the cache if needed.
@@ -83,29 +92,31 @@ void OSROptimizedCodeCache::Compact(Handle<NativeContext> native_context) {
   DCHECK_LT(new_osr_cache->length(), osr_cache->length());
   {
     DisallowGarbageCollection no_gc;
-    new_osr_cache->CopyElements(native_context->GetIsolate(), 0, *osr_cache, 0,
+    new_osr_cache->CopyElements(isolate, 0, *osr_cache, 0,
                                 new_osr_cache->length(),
                                 new_osr_cache->GetWriteBarrierMode(no_gc));
   }
   native_context->set_osr_code_cache(*new_osr_cache);
 }
 
-CodeT OSROptimizedCodeCache::GetOptimizedCode(SharedFunctionInfo shared,
-                                              BytecodeOffset osr_offset,
-                                              Isolate* isolate) {
+CodeT OSROptimizedCodeCache::TryGet(SharedFunctionInfo shared,
+                                    BytecodeOffset osr_offset,
+                                    Isolate* isolate) {
   DisallowGarbageCollection no_gc;
   int index = FindEntry(shared, osr_offset);
-  if (index == -1) return CodeT();
+  if (index == -1) return {};
+
   CodeT code = GetCodeFromEntry(index);
   if (code.is_null()) {
     ClearEntry(index, isolate);
-    return CodeT();
+    return {};
   }
+
   DCHECK(code.is_optimized_code() && !code.marked_for_deoptimization());
   return code;
 }
 
-void OSROptimizedCodeCache::EvictMarkedCode(Isolate* isolate) {
+void OSROptimizedCodeCache::EvictDeoptimizedCode(Isolate* isolate) {
   // This is called from DeoptimizeMarkedCodeForContext that uses raw pointers
   // and hence the DisallowGarbageCollection scope here.
   DisallowGarbageCollection no_gc;
@@ -135,9 +146,8 @@ std::vector<int> OSROptimizedCodeCache::GetBytecodeOffsetsFromSFI(
 }
 
 int OSROptimizedCodeCache::GrowOSRCache(
-    Handle<NativeContext> native_context,
+    Isolate* isolate, Handle<NativeContext> native_context,
     Handle<OSROptimizedCodeCache>* osr_cache) {
-  Isolate* isolate = native_context->GetIsolate();
   int old_length = (*osr_cache)->length();
   int grow_by = CapacityForLength(old_length) - old_length;
   DCHECK_GT(grow_by, kEntryLength);
@@ -256,5 +266,13 @@ bool OSROptimizedCodeCache::NeedsTrimming(int num_valid_entries,
   return curr_length > kInitialLength && curr_length > num_valid_entries * 3;
 }
 
+MaybeObject OSROptimizedCodeCache::RawGetForTesting(int index) const {
+  return WeakFixedArray::Get(index);
+}
+
+void OSROptimizedCodeCache::RawSetForTesting(int index, MaybeObject value) {
+  WeakFixedArray::Set(index, value);
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/osr-optimized-code-cache.h

@@ -6,6 +6,7 @@
 #define V8_OBJECTS_OSR_OPTIMIZED_CODE_CACHE_H_
 
 #include "src/objects/fixed-array.h"
+
 // Has to be the last include (doesn't have include guards):
 #include "src/objects/object-macros.h"
 
@@ -21,51 +22,79 @@ enum OSRCodeCacheStateOfSFI : uint8_t {
   kCachedMultiple,  // Very unlikely state, multiple entries.
 };
 
+// TODO(jgruber): There are a few issues with the current implementation:
+//
+// - The cache is a flat list, thus any search operation is O(N). This resulted
+//   in optimization attempts, see OSRCodeCacheStateOfSFI.
+// - We always iterate up to `length` (== capacity).
+// - We essentially reimplement WeakArrayList, i.e. growth and shrink logic.
+// - On overflow, new entries always pick slot 0.
+//
+// There are a few alternatives:
+//
+// 1) we could reuse WeakArrayList logic (but then we'd still have to
+//    implement custom compaction due to our entry tuple structure).
+// 2) we could reuse CompilationCacheTable (but then we lose weakness and have
+//    to deal with aging).
+// 3) we could try to base on a weak HashTable variant (EphemeronHashTable?).
 class V8_EXPORT OSROptimizedCodeCache : public WeakFixedArray {
  public:
   DECL_CAST(OSROptimizedCodeCache)
 
-  enum OSRCodeCacheConstants {
-    kSharedOffset,
-    kCachedCodeOffset,
-    kOsrIdOffset,
-    kEntryLength
-  };
-
-  static const int kInitialLength = OSRCodeCacheConstants::kEntryLength * 4;
-  static const int kMaxLength = OSRCodeCacheConstants::kEntryLength * 1024;
+  static Handle<OSROptimizedCodeCache> Empty(Isolate* isolate);
 
   // Caches the optimized code |code| corresponding to the shared function
   // |shared| and bailout id |osr_offset| in the OSROptimized code cache.
   // If the OSR code cache wasn't created before it creates a code cache with
   // kOSRCodeCacheInitialLength entries.
-  static void AddOptimizedCode(Handle<NativeContext> context,
-                               Handle<SharedFunctionInfo> shared,
-                               Handle<CodeT> code, BytecodeOffset osr_offset);
-  // Reduces the size of the OSR code cache if the number of valid entries are
-  // less than the current capacity of the cache.
-  static void Compact(Handle<NativeContext> context);
-  // Sets the OSR optimized code cache to an empty array.
-  static void Clear(NativeContext context);
+  static void Insert(Isolate* isolate, Handle<NativeContext> context,
+                     Handle<SharedFunctionInfo> shared, Handle<CodeT> code,
+                     BytecodeOffset osr_offset);
 
   // Returns the code corresponding to the shared function |shared| and
   // BytecodeOffset |offset| if an entry exists in the cache. Returns an empty
   // object otherwise.
-  CodeT GetOptimizedCode(SharedFunctionInfo shared, BytecodeOffset osr_offset,
-                         Isolate* isolate);
+  CodeT TryGet(SharedFunctionInfo shared, BytecodeOffset osr_offset,
+               Isolate* isolate);
 
   // Remove all code objects marked for deoptimization from OSR code cache.
-  void EvictMarkedCode(Isolate* isolate);
+  void EvictDeoptimizedCode(Isolate* isolate);
+
+  // Reduces the size of the OSR code cache if the number of valid entries are
+  // less than the current capacity of the cache.
+  static void Compact(Isolate* isolate, Handle<NativeContext> context);
+
+  // Sets the OSR optimized code cache to an empty array.
+  static void Clear(Isolate* isolate, NativeContext context);
 
   // Returns vector of bytecode offsets corresponding to the shared function
   // |shared|
   std::vector<int> GetBytecodeOffsetsFromSFI(SharedFunctionInfo shared);
 
+  enum OSRCodeCacheConstants {
+    kSharedOffset,
+    kCachedCodeOffset,
+    kOsrIdOffset,
+    kEntryLength
+  };
+
+  static constexpr int kInitialLength = OSRCodeCacheConstants::kEntryLength * 4;
+  static constexpr int kMaxLength = OSRCodeCacheConstants::kEntryLength * 1024;
+
+  // For osr-code-cache-unittest.cc.
+  MaybeObject RawGetForTesting(int index) const;
+  void RawSetForTesting(int index, MaybeObject value);
+
  private:
+  // Hide raw accessors to avoid terminology confusion.
+  using WeakFixedArray::Get;
+  using WeakFixedArray::Set;
+
   // Functions that implement heuristics on when to grow / shrink the cache.
   static int CapacityForLength(int curr_capacity);
   static bool NeedsTrimming(int num_valid_entries, int curr_capacity);
-  static int GrowOSRCache(Handle<NativeContext> native_context,
+  static int GrowOSRCache(Isolate* isolate,
+                          Handle<NativeContext> native_context,
                           Handle<OSROptimizedCodeCache>* osr_cache);
 
   // Helper functions to get individual items from an entry in the cache.