Ensure bytecode isn't flushed when allocating feedback vector

This is a followup of the cl [1] that fixes a bug where bytecode was
getting flushed when allocating feedback vector. The fix added
IsCompiledScope before allocating a new feedback vector. We now pass
IsCompiledScope to JSFunction::EnsureFeedbackVector. This makes it
explicit that EnsureFeedbackVector expects a function that is compiled
and the bytecode shouldn't be flushed during the allocation.Also adds
a test.


[1] https://chromium-review.googlesource.com/c/v8/v8/+/2218066

Bug: v8:10560
Change-Id: I552c449a57555dffa625b2e4efa04c2c276fc0b4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2222347
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68142}

src/codegen/compiler.cc

@@ -1595,7 +1595,7 @@ bool Compiler::Compile(Handle<JSFunction> function, ClearExceptionFlag flag,
   Handle<Code> code = handle(shared_info->GetCode(), isolate);
 
   // Initialize the feedback cell for this JSFunction.
-  JSFunction::InitializeFeedbackCell(function);
+  JSFunction::InitializeFeedbackCell(function, is_compiled_scope);
 
   // Optimize now if --always-opt is enabled.
   if (FLAG_always_opt && !function->shared().HasAsmWasmData()) {
@@ -1801,7 +1801,7 @@ MaybeHandle<JSFunction> Compiler::GetFunctionFromEval(
     } else {
       result = isolate->factory()->NewFunctionFromSharedFunctionInfo(
           shared_info, context, AllocationType::kYoung);
-      JSFunction::InitializeFeedbackCell(result);
+      JSFunction::InitializeFeedbackCell(result, &is_compiled_scope);
       if (allow_eval_cache) {
         // Make sure to cache this result.
         Handle<FeedbackCell> new_feedback_cell(result->raw_feedback_cell(),
@@ -1813,7 +1813,7 @@ MaybeHandle<JSFunction> Compiler::GetFunctionFromEval(
   } else {
     result = isolate->factory()->NewFunctionFromSharedFunctionInfo(
         shared_info, context, AllocationType::kYoung);
-    JSFunction::InitializeFeedbackCell(result);
+    JSFunction::InitializeFeedbackCell(result, &is_compiled_scope);
     if (allow_eval_cache) {
       // Add the SharedFunctionInfo and the LiteralsArray to the eval cache if
       // we didn't retrieve from there.
@@ -2764,7 +2764,7 @@ void Compiler::PostInstantiation(Handle<JSFunction> function) {
   // If code is compiled to bytecode (i.e., isn't asm.js), then allocate a
   // feedback and check for optimized code.
   if (is_compiled_scope.is_compiled() && shared->HasBytecodeArray()) {
-    JSFunction::InitializeFeedbackCell(function);
+    JSFunction::InitializeFeedbackCell(function, &is_compiled_scope);
 
     Code code = function->has_feedback_vector()
                     ? function->feedback_vector().optimized_code()
@@ -2779,7 +2779,7 @@ void Compiler::PostInstantiation(Handle<JSFunction> function) {
     if (FLAG_always_opt && shared->allows_lazy_compilation() &&
         !shared->optimization_disabled() && !function->IsOptimized() &&
         !function->HasOptimizedCode()) {
-      JSFunction::EnsureFeedbackVector(function);
+      JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
       function->MarkForOptimization(ConcurrencyMode::kNotConcurrent);
     }
   }

src/debug/debug-coverage.cc

@@ -793,7 +793,9 @@ void Coverage::SelectMode(Isolate* isolate, debug::CoverageMode mode) {
       }
 
       for (Handle<JSFunction> func : funcs_needing_feedback_vector) {
-        JSFunction::EnsureFeedbackVector(func);
+        IsCompiledScope is_compiled_scope(func->shared().is_compiled_scope());
+        CHECK(is_compiled_scope.is_compiled());
+        JSFunction::EnsureFeedbackVector(func, &is_compiled_scope);
       }
 
       // Root all feedback vectors to avoid early collection.

src/debug/liveedit.cc

@@ -1146,7 +1146,9 @@ void LiveEdit::PatchScript(Isolate* isolate, Handle<Script> script,
       js_function->set_raw_feedback_cell(
           *isolate->factory()->many_closures_cell());
       if (!js_function->is_compiled()) continue;
-      JSFunction::EnsureFeedbackVector(js_function);
+      IsCompiledScope is_compiled_scope(
+          js_function->shared().is_compiled_scope());
+      JSFunction::EnsureFeedbackVector(js_function, &is_compiled_scope);
     }
 
     if (!sfi->HasBytecodeArray()) continue;
@@ -1187,7 +1189,9 @@ void LiveEdit::PatchScript(Isolate* isolate, Handle<Script> script,
       js_function->set_raw_feedback_cell(
           *isolate->factory()->many_closures_cell());
       if (!js_function->is_compiled()) continue;
-      JSFunction::EnsureFeedbackVector(js_function);
+      IsCompiledScope is_compiled_scope(
+          js_function->shared().is_compiled_scope());
+      JSFunction::EnsureFeedbackVector(js_function, &is_compiled_scope);
     }
   }
   SharedFunctionInfo::ScriptIterator it(isolate, *new_script);

src/objects/feedback-vector.cc

@@ -235,13 +235,11 @@ Handle<ClosureFeedbackCellArray> ClosureFeedbackCellArray::New(
 // static
 Handle<FeedbackVector> FeedbackVector::New(
     Isolate* isolate, Handle<SharedFunctionInfo> shared,
-    Handle<ClosureFeedbackCellArray> closure_feedback_cell_array) {
+    Handle<ClosureFeedbackCellArray> closure_feedback_cell_array,
+    IsCompiledScope* is_compiled_scope) {
+  DCHECK(is_compiled_scope->is_compiled());
   Factory* factory = isolate->factory();
 
-  // Hold on to bytecode here. The allocation of a new feedback vector could
-  // trigger a GC and flush the bytecode and feedback metadata.
-  IsCompiledScope is_compiled_scope(*shared, isolate);
-  CHECK(is_compiled_scope.is_compiled());
   Handle<FeedbackMetadata> feedback_metadata(shared->feedback_metadata(),
                                              isolate);
   const int slot_count = feedback_metadata->slot_count();
@@ -342,7 +340,9 @@ Handle<FeedbackVector> FeedbackVector::NewWithOneCompareSlotForTesting(
   Handle<ClosureFeedbackCellArray> closure_feedback_cell_array =
       ClosureFeedbackCellArray::New(isolate, shared);
 
-  return FeedbackVector::New(isolate, shared, closure_feedback_cell_array);
+  IsCompiledScope is_compiled_scope(shared->is_compiled_scope());
+  return FeedbackVector::New(isolate, shared, closure_feedback_cell_array,
+                             &is_compiled_scope);
 }
 
 // static

src/objects/feedback-vector.h

@@ -23,6 +23,8 @@
 namespace v8 {
 namespace internal {
 
+class IsCompiledScope;
+
 enum class FeedbackSlotKind {
   // This kind means that the slot points to the middle of other slot
   // which occupies more than one feedback vector element.
@@ -149,11 +151,9 @@ using MaybeObjectHandles = std::vector<MaybeObjectHandle>;
 class FeedbackMetadata;
 
 // ClosureFeedbackCellArray is a FixedArray that contains feedback cells used
-// when creating closures from a function. Along with the feedback
-// cells, the first slot (slot 0) is used to hold a budget to measure the
-// hotness of the function. This is created once the function is compiled and is
-// either held by the feedback vector (if allocated) or by the FeedbackCell of
-// the closure.
+// when creating closures from a function. This is created once the function is
+// compiled and is either held by the feedback vector (if allocated) or by the
+// FeedbackCell of the closure.
 class ClosureFeedbackCellArray : public FixedArray {
  public:
   NEVER_READ_ONLY_SPACE
@@ -262,7 +262,8 @@ class FeedbackVector : public HeapObject {
 
   V8_EXPORT_PRIVATE static Handle<FeedbackVector> New(
       Isolate* isolate, Handle<SharedFunctionInfo> shared,
-      Handle<ClosureFeedbackCellArray> closure_feedback_cell_array);
+      Handle<ClosureFeedbackCellArray> closure_feedback_cell_array,
+      IsCompiledScope* is_compiled_scope);
 
   V8_EXPORT_PRIVATE static Handle<FeedbackVector>
   NewWithOneCompareSlotForTesting(Zone* zone, Isolate* isolate);

src/objects/js-objects.cc

@@ -4994,9 +4994,10 @@ void JSFunction::EnsureClosureFeedbackCellArray(Handle<JSFunction> function) {
 }
 
 // static
-void JSFunction::EnsureFeedbackVector(Handle<JSFunction> function) {
+void JSFunction::EnsureFeedbackVector(Handle<JSFunction> function,
+                                      IsCompiledScope* is_compiled_scope) {
   Isolate* const isolate = function->GetIsolate();
-  DCHECK(function->shared().is_compiled());
+  DCHECK(is_compiled_scope->is_compiled());
   DCHECK(function->shared().HasFeedbackMetadata());
   if (function->has_feedback_vector()) return;
   if (function->shared().HasAsmWasmData()) return;
@@ -5007,8 +5008,8 @@ void JSFunction::EnsureFeedbackVector(Handle<JSFunction> function) {
   EnsureClosureFeedbackCellArray(function);
   Handle<ClosureFeedbackCellArray> closure_feedback_cell_array =
       handle(function->closure_feedback_cell_array(), isolate);
-  Handle<HeapObject> feedback_vector =
-      FeedbackVector::New(isolate, shared, closure_feedback_cell_array);
+  Handle<HeapObject> feedback_vector = FeedbackVector::New(
+      isolate, shared, closure_feedback_cell_array, is_compiled_scope);
   // EnsureClosureFeedbackCellArray should handle the special case where we need
   // to allocate a new feedback cell. Please look at comment in that function
   // for more details.
@@ -5019,7 +5020,8 @@ void JSFunction::EnsureFeedbackVector(Handle<JSFunction> function) {
 }
 
 // static
-void JSFunction::InitializeFeedbackCell(Handle<JSFunction> function) {
+void JSFunction::InitializeFeedbackCell(Handle<JSFunction> function,
+                                        IsCompiledScope* is_compiled_scope) {
   Isolate* const isolate = function->GetIsolate();
 
   if (function->has_feedback_vector()) {
@@ -5037,7 +5039,7 @@ void JSFunction::InitializeFeedbackCell(Handle<JSFunction> function) {
   if (FLAG_always_opt) needs_feedback_vector = true;
 
   if (needs_feedback_vector) {
-    EnsureFeedbackVector(function);
+    EnsureFeedbackVector(function, is_compiled_scope);
   } else {
     EnsureClosureFeedbackCellArray(function);
   }

src/objects/js-objects.h

@@ -27,6 +27,7 @@ enum InstanceType : uint16_t;
 class JSGlobalObject;
 class JSGlobalProxy;
 class NativeContext;
+class IsCompiledScope;
 
 // JSReceiver includes types on which properties can be defined, i.e.,
 // JSObject and JSProxy.
@@ -1064,7 +1065,7 @@ class JSFunction : public JSFunctionOrBoundFunction {
   inline FeedbackVector feedback_vector() const;
   inline bool has_feedback_vector() const;
   V8_EXPORT_PRIVATE static void EnsureFeedbackVector(
-      Handle<JSFunction> function);
+      Handle<JSFunction> function, IsCompiledScope* compiled_scope);
 
   // Functions related to clousre feedback cell array that holds feedback cells
   // used to create closures from this function. We allocate closure feedback
@@ -1078,7 +1079,8 @@ class JSFunction : public JSFunctionOrBoundFunction {
   // initialized to the closure feedback cell array that holds the feedback
   // cells for create closure calls from this function. In the regular mode,
   // this allocates feedback vector.
-  static void InitializeFeedbackCell(Handle<JSFunction> function);
+  static void InitializeFeedbackCell(Handle<JSFunction> function,
+                                     IsCompiledScope* compiled_scope);
 
   // Unconditionally clear the type feedback vector.
   void ClearTypeFeedbackInfo();

src/runtime/runtime-internal.cc

@@ -332,7 +332,8 @@ RUNTIME_FUNCTION(Runtime_BytecodeBudgetInterrupt) {
   CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 0);
   function->raw_feedback_cell().set_interrupt_budget(FLAG_interrupt_budget);
   if (!function->has_feedback_vector()) {
-    JSFunction::EnsureFeedbackVector(function);
+    IsCompiledScope is_compiled_scope(function->shared().is_compiled_scope());
+    JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
     // Also initialize the invocation count here. This is only really needed for
     // OSR. When we OSR functions with lazy feedback allocation we want to have
     // a non zero invocation count so we can inline functions.

src/runtime/runtime-test.cc

@@ -323,7 +323,7 @@ RUNTIME_FUNCTION(Runtime_OptimizeFunctionOnNextCall) {
     function->set_code(*BUILTIN_CODE(isolate, InterpreterEntryTrampoline));
   }
 
-  JSFunction::EnsureFeedbackVector(function);
+  JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
   function->MarkForOptimization(concurrency_mode);
 
   return ReadOnlyRoots(isolate).undefined_value();
@@ -353,7 +353,7 @@ bool EnsureFeedbackVector(Handle<JSFunction> function) {
 
   // Ensure function has a feedback vector to hold type feedback for
   // optimization.
-  JSFunction::EnsureFeedbackVector(function);
+  JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
   return true;
 }
 
@@ -457,7 +457,8 @@ RUNTIME_FUNCTION(Runtime_OptimizeOsr) {
     function->ShortPrint(scope.file());
     PrintF(scope.file(), " for non-concurrent optimization]\n");
   }
-  JSFunction::EnsureFeedbackVector(function);
+  IsCompiledScope is_compiled_scope(function->shared().is_compiled_scope());
+  JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
   function->MarkForOptimization(ConcurrencyMode::kNotConcurrent);
 
   // Make the profiler arm all back edges in unoptimized code.

test/cctest/cctest.cc

@@ -262,7 +262,7 @@ i::Handle<i::JSFunction> Optimize(
   }
 
   CHECK(info.shared_info()->HasBytecodeArray());
-  i::JSFunction::EnsureFeedbackVector(function);
+  i::JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
 
   i::Handle<i::Code> code =
       i::compiler::Pipeline::GenerateCodeForTesting(&info, isolate, out_broker)

test/cctest/compiler/test-run-bytecode-graph-builder.cc

@@ -115,7 +115,8 @@ class BytecodeGraphTester {
             .ToLocalChecked());
     Handle<JSFunction> function =
         Handle<JSFunction>::cast(v8::Utils::OpenHandle(*api_function));
-    JSFunction::EnsureFeedbackVector(function);
+    IsCompiledScope is_compiled_scope(function->shared().is_compiled_scope());
+    JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
     CHECK(function->shared().HasBytecodeArray());
 
     Zone zone(isolate_->allocator(), ZONE_NAME);

test/cctest/heap/heap-tester.h

@@ -48,6 +48,7 @@
   V(TestMemoryReducerSampleJsCalls)                         \
   V(TestSizeOfObjects)                                      \
   V(Regress5831)                                            \
+  V(Regress10560)                                           \
   V(Regress538257)                                          \
   V(Regress587004)                                          \
   V(Regress589413)                                          \

test/cctest/heap/test-heap.cc

@@ -1184,6 +1184,78 @@ TEST(TestBytecodeFlushing) {
   }
 }
 
+HEAP_TEST(Regress10560) {
+  i::FLAG_flush_bytecode = true;
+  i::FLAG_allow_natives_syntax = true;
+  // Disable flags that allocate a feedback vector eagerly.
+  i::FLAG_opt = false;
+  i::FLAG_always_opt = false;
+  i::FLAG_lazy_feedback_allocation = true;
+
+  ManualGCScope manual_gc_scope;
+  CcTest::InitializeVM();
+  v8::Isolate* isolate = CcTest::isolate();
+  Isolate* i_isolate = CcTest::i_isolate();
+  Factory* factory = i_isolate->factory();
+  Heap* heap = i_isolate->heap();
+
+  {
+    v8::HandleScope scope(isolate);
+    const char* source =
+        "function foo() {"
+        "  var x = 42;"
+        "  var y = 42;"
+        "  var z = x + y;"
+        "};"
+        "foo()";
+    Handle<String> foo_name = factory->InternalizeUtf8String("foo");
+    CompileRun(source);
+
+    // Check function is compiled.
+    Handle<Object> func_value =
+        Object::GetProperty(i_isolate, i_isolate->global_object(), foo_name)
+            .ToHandleChecked();
+    CHECK(func_value->IsJSFunction());
+    Handle<JSFunction> function = Handle<JSFunction>::cast(func_value);
+    CHECK(function->shared().is_compiled());
+    CHECK(!function->has_feedback_vector());
+
+    // Pre-age bytecode so it will be flushed on next run.
+    CHECK(function->shared().HasBytecodeArray());
+    const int kAgingThreshold = 6;
+    for (int i = 0; i < kAgingThreshold; i++) {
+      function->shared().GetBytecodeArray().MakeOlder();
+      if (function->shared().GetBytecodeArray().IsOld()) break;
+    }
+
+    CHECK(function->shared().GetBytecodeArray().IsOld());
+
+    heap::SimulateFullSpace(heap->old_space());
+
+    // Just check bytecode isn't flushed still
+    CHECK(function->shared().GetBytecodeArray().IsOld());
+    CHECK(function->shared().is_compiled());
+
+    heap->set_force_oom(true);
+    heap->AddNearHeapLimitCallback(
+        [](void* data, size_t current_heap_limit,
+           size_t initial_heap_limit) -> size_t {
+          Heap* heap = static_cast<Heap*>(data);
+          heap->set_force_oom(false);
+          return 0;
+        },
+        heap);
+
+    // Allocate feedback vector.
+    IsCompiledScope is_compiled_scope(function->shared().is_compiled_scope());
+    JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
+
+    CHECK(function->has_feedback_vector());
+    CHECK(function->shared().is_compiled());
+    CHECK(function->is_compiled());
+  }
+}
+
 #ifndef V8_LITE_MODE
 
 TEST(TestOptimizeAfterBytecodeFlushingCandidate) {

test/cctest/interpreter/interpreter-tester.h

@@ -127,6 +127,7 @@ class InterpreterTester {
   template <class... A>
   Handle<JSFunction> GetBytecodeFunction() {
     Handle<JSFunction> function;
+    IsCompiledScope is_compiled_scope;
     if (source_) {
       CompileRun(source_);
       v8::Local<v8::Context> context =
@@ -136,6 +137,7 @@ class InterpreterTester {
                                     ->Get(context, v8_str(kFunctionName))
                                     .ToLocalChecked());
       function = Handle<JSFunction>::cast(v8::Utils::OpenHandle(*api_function));
+      is_compiled_scope = function->shared().is_compiled_scope();
     } else {
       int arg_count = sizeof...(A);
       std::string source("(function " + function_name() + "(");
@@ -146,10 +148,12 @@ class InterpreterTester {
       function = Handle<JSFunction>::cast(v8::Utils::OpenHandle(
           *v8::Local<v8::Function>::Cast(CompileRun(source.c_str()))));
       function->set_code(*BUILTIN_CODE(isolate_, InterpreterEntryTrampoline));
+      is_compiled_scope = function->shared().is_compiled_scope();
     }
 
     if (!bytecode_.is_null()) {
       function->shared().set_function_data(*bytecode_.ToHandleChecked());
+      is_compiled_scope = function->shared().is_compiled_scope();
     }
     if (HasFeedbackMetadata()) {
       function->set_raw_feedback_cell(isolate_->heap()->many_closures_cell());
@@ -157,7 +161,7 @@ class InterpreterTester {
       // overwriting existing metadata.
       function->shared().set_raw_outer_scope_info_or_feedback_metadata(
           *feedback_metadata_.ToHandleChecked());
-      JSFunction::EnsureFeedbackVector(function);
+      JSFunction::EnsureFeedbackVector(function, &is_compiled_scope);
     }
     return function;
   }

test/cctest/test-feedback-vector.h

@@ -51,7 +51,9 @@ Handle<FeedbackVector> NewFeedbackVector(Isolate* isolate, Spec* spec) {
   shared->set_raw_outer_scope_info_or_feedback_metadata(*metadata);
   Handle<ClosureFeedbackCellArray> closure_feedback_cell_array =
       ClosureFeedbackCellArray::New(isolate, shared);
-  return FeedbackVector::New(isolate, shared, closure_feedback_cell_array);
+  IsCompiledScope is_compiled_scope(shared->is_compiled_scope());
+  return FeedbackVector::New(isolate, shared, closure_feedback_cell_array,
+                             &is_compiled_scope);
 }
 
 template <typename Spec>

test/unittests/compiler/js-call-reducer-unittest.cc

@@ -112,8 +112,9 @@ class JSCallReducerTest : public TypedGraphTest {
     shared->set_raw_outer_scope_info_or_feedback_metadata(*metadata);
     Handle<ClosureFeedbackCellArray> closure_feedback_cell_array =
         ClosureFeedbackCellArray::New(isolate(), shared);
-    Handle<FeedbackVector> vector =
-        FeedbackVector::New(isolate(), shared, closure_feedback_cell_array);
+    IsCompiledScope is_compiled_scope(shared->is_compiled_scope());
+    Handle<FeedbackVector> vector = FeedbackVector::New(
+        isolate(), shared, closure_feedback_cell_array, &is_compiled_scope);
     FeedbackSource feedback(vector, FeedbackSlot(0));
     return javascript()->Call(
         arity, CallFrequency(), feedback, ConvertReceiverMode::kAny,

test/unittests/compiler/redundancy-elimination-unittest.cc

@@ -39,8 +39,9 @@ class RedundancyEliminationTest : public GraphTest {
     shared->set_raw_outer_scope_info_or_feedback_metadata(*metadata);
     Handle<ClosureFeedbackCellArray> closure_feedback_cell_array =
         ClosureFeedbackCellArray::New(isolate(), shared);
-    Handle<FeedbackVector> feedback_vector =
-        FeedbackVector::New(isolate(), shared, closure_feedback_cell_array);
+    IsCompiledScope is_compiled_scope(shared->is_compiled_scope());
+    Handle<FeedbackVector> feedback_vector = FeedbackVector::New(
+        isolate(), shared, closure_feedback_cell_array, &is_compiled_scope);
     vector_slot_pairs_.push_back(FeedbackSource());
     vector_slot_pairs_.push_back(FeedbackSource(feedback_vector, slot1));
     vector_slot_pairs_.push_back(FeedbackSource(feedback_vector, slot2));