Only flush feedback vector on bytecode flush

When bytecode is flushed we also want to flush the feedback vectors to
save memory. There was a bug in this code and we flushed
ClosureFeedbackCellArray too. Flushing ClosureFeedbackCellArrays causes
the closures created by this function before and after the bytecode
flush to have different feedback cells and hence different feedback
vectors. This cl fixes it so we only flush feedback vectors on a
bytecode flush.

Also this cl pretenures ClosureFeedbackCellArrays. Only FeedbackCells
and FeedbackVectors can contain ClosureFeedbackCellArrays which are
pretenured, so it is better to pretenure ClosureFeedbackCellArrays as
well.


Bug: chromium:1031479
Change-Id: I7831441a95420b9e5711f4143461f1eb7fa1616a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1980582
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65866}

src/heap/factory.cc

@@ -509,7 +509,7 @@ Handle<ClosureFeedbackCellArray> Factory::NewClosureFeedbackCellArray(
   Handle<ClosureFeedbackCellArray> feedback_cell_array =
       NewFixedArrayWithMap<ClosureFeedbackCellArray>(
           RootIndex::kClosureFeedbackCellArrayMap, length,
-          AllocationType::kYoung);
+          AllocationType::kOld);
 
   return feedback_cell_array;
 }

src/heap/mark-compact.cc

@@ -2292,7 +2292,11 @@ void MarkCompactCollector::ClearFlushedJsFunctions() {
   JSFunction flushed_js_function;
   while (weak_objects_.flushed_js_functions.Pop(kMainThreadTask,
                                                 &flushed_js_function)) {
-    flushed_js_function.ResetIfBytecodeFlushed();
+    auto gc_notify_updated_slot = [](HeapObject object, ObjectSlot slot,
+                                     Object target) {
+      RecordSlot(object, slot, HeapObject::cast(target));
+    };
+    flushed_js_function.ResetIfBytecodeFlushed(gc_notify_updated_slot);
   }
 }
 

src/objects/feedback-cell-inl.h

@@ -26,9 +26,21 @@ void FeedbackCell::clear_padding() {
          FeedbackCell::kAlignedSize - FeedbackCell::kUnalignedSize);
 }
 
-void FeedbackCell::reset() {
-  set_value(GetReadOnlyRoots().undefined_value());
+void FeedbackCell::reset_feedback_vector(
+    base::Optional<std::function<void(HeapObject object, ObjectSlot slot,
+                                      HeapObject target)>>
+        gc_notify_updated_slot) {
   set_interrupt_budget(FeedbackCell::GetInitialInterruptBudget());
+  if (value().IsUndefined() || value().IsClosureFeedbackCellArray()) return;
+
+  CHECK(value().IsFeedbackVector());
+  ClosureFeedbackCellArray closure_feedback_cell_array =
+      FeedbackVector::cast(value()).closure_feedback_cell_array();
+  set_value(closure_feedback_cell_array);
+  if (gc_notify_updated_slot) {
+    (*gc_notify_updated_slot)(*this, RawField(FeedbackCell::kValueOffset),
+                              closure_feedback_cell_array);
+  }
 }
 
 }  // namespace internal

src/objects/feedback-cell.h

@@ -34,7 +34,10 @@ class FeedbackCell : public TorqueGeneratedFeedbackCell<FeedbackCell, Struct> {
   static const int kAlignedSize = RoundUp<kObjectAlignment>(int{kSize});
 
   inline void clear_padding();
-  inline void reset();
+  inline void reset_feedback_vector(
+      base::Optional<std::function<void(HeapObject object, ObjectSlot slot,
+                                        HeapObject target)>>
+          gc_notify_updated_slot = base::nullopt);
 
   using BodyDescriptor =
       FixedBodyDescriptor<kValueOffset, kInterruptBudgetOffset, kAlignedSize>;

src/objects/js-objects-inl.h

@@ -705,12 +705,15 @@ bool JSFunction::NeedsResetDueToFlushedBytecode() {
          code.builtin_index() != Builtins::kCompileLazy;
 }
 
-void JSFunction::ResetIfBytecodeFlushed() {
+void JSFunction::ResetIfBytecodeFlushed(
+    base::Optional<std::function<void(HeapObject object, ObjectSlot slot,
+                                      HeapObject target)>>
+        gc_notify_updated_slot) {
   if (FLAG_flush_bytecode && NeedsResetDueToFlushedBytecode()) {
     // Bytecode was flushed and function is now uncompiled, reset JSFunction
     // by setting code to CompileLazy and clearing the feedback vector.
     set_code(GetIsolate()->builtins()->builtin(i::Builtins::kCompileLazy));
-    raw_feedback_cell().reset();
+    raw_feedback_cell().reset_feedback_vector(gc_notify_updated_slot);
   }
 }
 

src/objects/js-objects.h

@@ -1081,7 +1081,10 @@ class JSFunction : public JSFunctionOrBoundFunction {
 
   // Resets function to clear compiled data after bytecode has been flushed.
   inline bool NeedsResetDueToFlushedBytecode();
-  inline void ResetIfBytecodeFlushed();
+  inline void ResetIfBytecodeFlushed(
+      base::Optional<std::function<void(HeapObject object, ObjectSlot slot,
+                                        HeapObject target)>>
+          gc_notify_updated_slot = base::nullopt);
 
   DECL_GETTER(has_prototype_slot, bool)
 

src/runtime/runtime-compiler.cc

@@ -188,18 +188,11 @@ static bool IsSuitableForOnStackReplacement(Isolate* isolate,
                                             Handle<JSFunction> function) {
   // Keep track of whether we've succeeded in optimizing.
   if (function->shared().optimization_disabled()) return false;
-  // TODO(chromium:1031479): When we flush the bytecode we also reset the
-  // feedback_cell that holds feedback vector / closure feedback cell array.
-  // Closure feedback cell array holds the feedback cells that should be used
-  // when creating closures from the given function. If we happen to execute the
-  // function again, we re-compile and re-intialize this array. So, the closures
-  // created by this function after the flush would have different feedback
-  // cells when compared to the corresponding closures created before the flush.
-  // Currently, OSR triggering mechanism is tied to the bytecode array. So, it
-  // is possible to mark one closure for OSR but optimize a different one.
-  // These two factors could cause us to OSR functions that don't have feedback
-  // vector. This is a temporary fix and we should fix one (or both) of the two
-  // causes.
+  // TODO(chromium:1031479): Currently, OSR triggering mechanism is tied to the
+  // bytecode array. So, it might be possible to mark closure in one native
+  // context and optimize a closure from a different native context. So check if
+  // there is a feedback vector before OSRing. We don't expect this to happen
+  // often.
   if (!function->has_feedback_vector()) return false;
   // If we are trying to do OSR when there are already optimized
   // activations of the function, it means (a) the function is directly or