Revert "Refactor OSROptimizedCodeCache"

This reverts commit d368dcf4.

Reason for revert: https://crbug.com/1312188

Original change's description:
> 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).
>
> Bug: v8:12161
> Change-Id: Ib34e372aa58a30c68c9c5cdd0d1da0ec3e86717c
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3560447
> Reviewed-by: Leszek Swirski <leszeks@chromium.org>
> Commit-Queue: Jakob Linke <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#79687}

Bug: v8:12161, chromium:1312188
Change-Id: Ieb3a91682845a23536fdfdf3208af74b3c6585f8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3564989
Auto-Submit: Adam Klein <adamk@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#79700}

src/codegen/compiler.cc

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

src/deoptimizer/deoptimizer.cc

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

src/execution/tiering-manager.cc

@@ -156,7 +156,8 @@ 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().osr_code_cache();
+    OSROptimizedCodeCache cache =
+        function.native_context().GetOSROptimizedCodeCache();
     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(*OSROptimizedCodeCache::Empty(isolate()));
+  context.set_osr_code_cache(*empty_weak_fixed_array());
   context.set_retained_maps(*empty_weak_array_list());
   return handle(context, isolate());
 }

src/objects/contexts-inl.h

@@ -296,6 +296,10 @@ 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, OSROptimizedCodeCache, osr_code_cache)
+  V(OSR_CODE_CACHE_INDEX, WeakFixedArray, osr_code_cache)
 
 #include "torque-generated/src/objects/contexts-tq.inc"
 
@@ -777,6 +777,8 @@ 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,28 +12,22 @@
 namespace v8 {
 namespace internal {
 
-// static
-Handle<OSROptimizedCodeCache> OSROptimizedCodeCache::Empty(Isolate* isolate) {
-  return Handle<OSROptimizedCodeCache>::cast(
-      isolate->factory()->empty_weak_fixed_array());
-}
+const int OSROptimizedCodeCache::kInitialLength;
+const int OSROptimizedCodeCache::kMaxLength;
 
-// static
-void OSROptimizedCodeCache::Insert(Isolate* isolate,
-                                   Handle<NativeContext> native_context,
-                                   Handle<SharedFunctionInfo> shared,
-                                   Handle<CodeT> code,
-                                   BytecodeOffset osr_offset) {
+void OSROptimizedCodeCache::AddOptimizedCode(
+    Handle<NativeContext> native_context, Handle<SharedFunctionInfo> shared,
+    Handle<CodeT> code, BytecodeOffset osr_offset) {
   DCHECK(!osr_offset.IsNone());
-  DCHECK(!isolate->serializer_enabled());
   DCHECK(CodeKindIsOptimizedJSFunction(code->kind()));
+  STATIC_ASSERT(kEntryLength == 3);
+  Isolate* isolate = native_context->GetIsolate();
+  DCHECK(!isolate->serializer_enabled());
 
-  Handle<OSROptimizedCodeCache> osr_cache(native_context->osr_code_cache(),
-                                          isolate);
+  Handle<OSROptimizedCodeCache> osr_cache(
+      native_context->GetOSROptimizedCodeCache(), 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() ||
@@ -43,31 +37,28 @@ void OSROptimizedCodeCache::Insert(Isolate* isolate,
     }
   }
 
-  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.
+  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;
   }
-  }
 
   osr_cache->InitializeEntry(entry, *shared, *code, osr_offset);
 }
 
-void OSROptimizedCodeCache::Clear(Isolate* isolate,
-                                  NativeContext native_context) {
-  native_context.set_osr_code_cache(*OSROptimizedCodeCache::Empty(isolate));
+void OSROptimizedCodeCache::Clear(NativeContext native_context) {
+  native_context.set_osr_code_cache(
+      *native_context.GetIsolate()->factory()->empty_weak_fixed_array());
 }
 
-void OSROptimizedCodeCache::Compact(Isolate* isolate,
-                                    Handle<NativeContext> native_context) {
-  Handle<OSROptimizedCodeCache> osr_cache(native_context->osr_code_cache(),
-                                          isolate);
+void OSROptimizedCodeCache::Compact(Handle<NativeContext> native_context) {
+  Handle<OSROptimizedCodeCache> osr_cache(
+      native_context->GetOSROptimizedCodeCache(), native_context->GetIsolate());
+  Isolate* isolate = native_context->GetIsolate();
 
   // Re-adjust the cache so all the valid entries are on one side. This will
   // enable us to compress the cache if needed.
@@ -92,31 +83,29 @@ void OSROptimizedCodeCache::Compact(Isolate* isolate,
   DCHECK_LT(new_osr_cache->length(), osr_cache->length());
   {
     DisallowGarbageCollection no_gc;
-    new_osr_cache->CopyElements(isolate, 0, *osr_cache, 0,
+    new_osr_cache->CopyElements(native_context->GetIsolate(), 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::TryGet(SharedFunctionInfo shared,
+CodeT OSROptimizedCodeCache::GetOptimizedCode(SharedFunctionInfo shared,
                                               BytecodeOffset osr_offset,
                                               Isolate* isolate) {
   DisallowGarbageCollection no_gc;
   int index = FindEntry(shared, osr_offset);
-  if (index == -1) return {};
-
+  if (index == -1) return CodeT();
   CodeT code = GetCodeFromEntry(index);
   if (code.is_null()) {
     ClearEntry(index, isolate);
-    return {};
+    return CodeT();
   }
-
   DCHECK(code.is_optimized_code() && !code.marked_for_deoptimization());
   return code;
 }
 
-void OSROptimizedCodeCache::EvictDeoptimizedCode(Isolate* isolate) {
+void OSROptimizedCodeCache::EvictMarkedCode(Isolate* isolate) {
   // This is called from DeoptimizeMarkedCodeForContext that uses raw pointers
   // and hence the DisallowGarbageCollection scope here.
   DisallowGarbageCollection no_gc;
@@ -146,8 +135,9 @@ std::vector<int> OSROptimizedCodeCache::GetBytecodeOffsetsFromSFI(
 }
 
 int OSROptimizedCodeCache::GrowOSRCache(
-    Isolate* isolate, Handle<NativeContext> native_context,
+    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);
@@ -266,13 +256,5 @@ 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,7 +6,6 @@
 #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"
 
@@ -22,79 +21,51 @@ 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)
 
-  static Handle<OSROptimizedCodeCache> Empty(Isolate* isolate);
+  enum OSRCodeCacheConstants {
+    kSharedOffset,
+    kCachedCodeOffset,
+    kOsrIdOffset,
+    kEntryLength
+  };
+
+  static const int kInitialLength = OSRCodeCacheConstants::kEntryLength * 4;
+  static const int kMaxLength = OSRCodeCacheConstants::kEntryLength * 1024;
 
   // 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 Insert(Isolate* isolate, Handle<NativeContext> context,
-                     Handle<SharedFunctionInfo> shared, Handle<CodeT> code,
-                     BytecodeOffset osr_offset);
+  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);
 
   // 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 TryGet(SharedFunctionInfo shared, BytecodeOffset osr_offset,
+  CodeT GetOptimizedCode(SharedFunctionInfo shared, BytecodeOffset osr_offset,
                          Isolate* isolate);
 
   // Remove all code objects marked for deoptimization from OSR code cache.
-  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);
+  void EvictMarkedCode(Isolate* isolate);
 
   // 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(Isolate* isolate,
-                          Handle<NativeContext> native_context,
+  static int GrowOSRCache(Handle<NativeContext> native_context,
                           Handle<OSROptimizedCodeCache>* osr_cache);
 
   // Helper functions to get individual items from an entry in the cache.