[Compile] Avoid flushing code that's marked for optimization in tests.

Bytecode flushing can make tests using assertOptimized flaky if the bytecode is
flushed between marking and optimization. It can also be flaky if the feedback vector
is collected before optimization. To prevent this, a new %PrepareForOptimization
runtime-test function is added that hold onto the bytecode strongly until it is
optimized after being explicitly marked for optimization by %OptimizeFunctionOnNextCall.

BUG=v8:8801,v8:8395

Change-Id: Idbd962a3a2044b915903f9c5e92d1789942b5b41
Reviewed-on: https://chromium-review.googlesource.com/c/1463525
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59914}

src/compiler.cc

@@ -22,7 +22,7 @@
 #include "src/debug/liveedit.h"
 #include "src/frames-inl.h"
 #include "src/globals.h"
-#include "src/heap/heap.h"
+#include "src/heap/heap-inl.h"
 #include "src/interpreter/interpreter.h"
 #include "src/isolate-inl.h"
 #include "src/log-inl.h"
@@ -758,6 +758,15 @@ MaybeHandle<Code> GetOptimizedCode(Handle<JSFunction> function,
     return MaybeHandle<Code>();
   }
 
+  // If code was pending optimization for testing, delete remove the strong root
+  // that was preventing the bytecode from being flushed between marking and
+  // optimization.
+  if (isolate->heap()->pending_optimize_for_test_bytecode() ==
+      shared->GetBytecodeArray()) {
+    isolate->heap()->SetPendingOptimizeForTestBytecode(
+        ReadOnlyRoots(isolate).undefined_value());
+  }
+
   Handle<Code> cached_code;
   if (GetCodeFromOptimizedCodeCache(function, osr_offset)
           .ToHandle(&cached_code)) {

src/heap/heap-inl.h

@@ -119,6 +119,11 @@ void Heap::SetMessageListeners(TemplateList value) {
   roots_table()[RootIndex::kMessageListeners] = value->ptr();
 }
 
+void Heap::SetPendingOptimizeForTestBytecode(Object bytecode) {
+  DCHECK(bytecode->IsBytecodeArray() || bytecode->IsUndefined(isolate()));
+  roots_table()[RootIndex::kPendingOptimizeForTestBytecode] = bytecode->ptr();
+}
+
 PagedSpace* Heap::paged_space(int idx) {
   DCHECK_NE(idx, LO_SPACE);
   DCHECK_NE(idx, NEW_SPACE);

src/heap/heap.h

@@ -660,6 +660,7 @@ class Heap {
   V8_INLINE void SetRootStringTable(StringTable value);
   V8_INLINE void SetRootNoScriptSharedFunctionInfos(Object value);
   V8_INLINE void SetMessageListeners(TemplateList value);
+  V8_INLINE void SetPendingOptimizeForTestBytecode(Object bytecode);
 
   // Set the stack limit in the roots table.  Some architectures generate
   // code that looks here, because it is faster than loading from the static

src/heap/setup-heap-internal.cc

@@ -792,6 +792,7 @@ void Heap::CreateInitialObjects() {
   set_retaining_path_targets(roots.empty_weak_array_list());
 
   set_feedback_vectors_for_profiling_tools(roots.undefined_value());
+  set_pending_optimize_for_test_bytecode(roots.undefined_value());
 
   set_script_list(roots.empty_weak_array_list());
 

src/roots.h

@@ -290,7 +290,8 @@ class RootVisitor;
   /* KeepDuringJob set for JS WeakRefs */                                  \
   V(HeapObject, weak_refs_keep_during_job, WeakRefsKeepDuringJob)          \
   V(HeapObject, interpreter_entry_trampoline_for_profiling,                \
-    InterpreterEntryTrampolineForProfiling)
+    InterpreterEntryTrampolineForProfiling)                                \
+  V(Object, pending_optimize_for_test_bytecode, PendingOptimizeForTestBytecode)
 
 // Entries in this list are limited to Smis and are not visited during GC.
 #define SMI_ROOT_LIST(V)                                                       \

src/runtime/runtime-test.cc

@@ -296,6 +296,60 @@ RUNTIME_FUNCTION(Runtime_OptimizeFunctionOnNextCall) {
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
+RUNTIME_FUNCTION(Runtime_PrepareFunctionForOptimization) {
+  HandleScope scope(isolate);
+  DCHECK_EQ(1, args.length());
+  CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 0);
+
+  // Only one function should be prepared for optimization at a time
+  CHECK(isolate->heap()->pending_optimize_for_test_bytecode()->IsUndefined());
+
+  // Check function allows lazy compilation.
+  if (!function->shared()->allows_lazy_compilation()) {
+    return ReadOnlyRoots(isolate).undefined_value();
+  }
+
+  // If function isn't compiled, compile it now.
+  IsCompiledScope is_compiled_scope(function->shared()->is_compiled_scope());
+  if (!is_compiled_scope.is_compiled() &&
+      !Compiler::Compile(function, Compiler::CLEAR_EXCEPTION,
+                         &is_compiled_scope)) {
+    return ReadOnlyRoots(isolate).undefined_value();
+  }
+
+  // Ensure function has a feedback vector to hold type feedback for
+  // optimization.
+  JSFunction::EnsureFeedbackVector(function);
+
+  // If optimization is disabled for the function, return without making it
+  // pending optimize for test.
+  if (function->shared()->optimization_disabled() &&
+      function->shared()->disable_optimization_reason() ==
+          BailoutReason::kNeverOptimize) {
+    return ReadOnlyRoots(isolate).undefined_value();
+  }
+
+  // If the function is already optimized, return without making it pending
+  // optimize for test.
+  if (function->IsOptimized() || function->shared()->HasAsmWasmData()) {
+    return ReadOnlyRoots(isolate).undefined_value();
+  }
+
+  // If the function has optimized code, ensure that we check for it and then
+  // return without making it pending optimize for test.
+  if (function->HasOptimizedCode()) {
+    DCHECK(function->ChecksOptimizationMarker());
+    return ReadOnlyRoots(isolate).undefined_value();
+  }
+
+  // Hold onto the bytecode array between marking and optimization to ensure
+  // it's not flushed.
+  isolate->heap()->SetPendingOptimizeForTestBytecode(
+      function->shared()->GetBytecodeArray());
+
+  return ReadOnlyRoots(isolate).undefined_value();
+}
+
 RUNTIME_FUNCTION(Runtime_OptimizeOsr) {
   HandleScope scope(isolate);
   DCHECK(args.length() == 0 || args.length() == 1);

src/runtime/runtime.h

@@ -498,6 +498,7 @@ namespace internal {
   F(NotifyContextDisposed, 0, 1)              \
   F(OptimizeFunctionOnNextCall, -1, 1)        \
   F(OptimizeOsr, -1, 1)                       \
+  F(PrepareFunctionForOptimization, 1, 1)     \
   F(PrintWithNameForAssert, 2, 1)             \
   F(RedirectToWasmInterpreter, 2, 1)          \
   F(RunningInSimulator, 0, 1)                 \

test/mjsunit/array-push5.js

@@ -14,6 +14,7 @@ my_array_proto.__proto__ = [].__proto__;
 function push_wrapper_2(array, value) {
   array.push(value);
 }
+%PrepareFunctionForOptimization(push_wrapper_2);
 array = [];
 array.__proto__ = my_array_proto;
 push_wrapper_2(array, 66);

test/mjsunit/compiler/deopt-array-push.js

@@ -7,10 +7,12 @@
 (function test() {
   function foo(a) { a.push(a.length = 2); }
 
+  %PrepareFunctionForOptimization(foo);
   foo([1]);
   foo([1]);
   %OptimizeFunctionOnNextCall(foo);
   foo([1]);
+  %PrepareFunctionForOptimization(foo);
   %OptimizeFunctionOnNextCall(foo);
   foo([1]);
   assertOptimized(foo);
@@ -19,10 +21,12 @@
 (function testElementTypeCheckSmi() {
   function foo(a) { a.push('a'); }
 
+  %PrepareFunctionForOptimization(foo);
   foo([1]);
   foo([1]);
   %OptimizeFunctionOnNextCall(foo);
   foo([1]);
+  %PrepareFunctionForOptimization(foo);
   %OptimizeFunctionOnNextCall(foo);
   foo([1]);
   assertOptimized(foo);
@@ -31,10 +35,12 @@
 (function testElementTypeCheckDouble() {
   function foo(a) { a.push('a'); }
 
+  %PrepareFunctionForOptimization(foo);
   foo([0.3413312]);
   foo([0.3413312]);
   %OptimizeFunctionOnNextCall(foo);
   foo([0.3413312]);
+  %PrepareFunctionForOptimization(foo);
   %OptimizeFunctionOnNextCall(foo);
   foo([0.3413312]);
   assertOptimized(foo);
@@ -44,10 +50,12 @@
   %NeverOptimizeFunction(bar);
   function foo(a) { a.push(bar(a)); }
 
+  %PrepareFunctionForOptimization(foo);
   foo(["1"]);
   foo(["1"]);
   %OptimizeFunctionOnNextCall(foo);
   foo(["1"]);
+  %PrepareFunctionForOptimization(foo);
   %OptimizeFunctionOnNextCall(foo);
   foo(["1"]);
   assertOptimized(foo);
@@ -56,10 +64,12 @@
 (function test() {
   function foo(a) { a.push(a.length = 2); }
 
+  %PrepareFunctionForOptimization(foo);
   foo([0.34234]);
   foo([0.34234]);
   %OptimizeFunctionOnNextCall(foo);
   foo([0.34234]);
+  %PrepareFunctionForOptimization(foo);
   %OptimizeFunctionOnNextCall(foo);
   foo([0.34234]);
   assertOptimized(foo);
@@ -70,10 +80,12 @@
 
   function foo(a) { a.push(1); }
 
+  %PrepareFunctionForOptimization(foo);
   foo(new Array(N));
   foo(new Array(N));
   %OptimizeFunctionOnNextCall(foo);
   foo(new Array(N));
+  %PrepareFunctionForOptimization(foo);
   %OptimizeFunctionOnNextCall(foo);
   foo(new Array(N));
   assertOptimized(foo);
@@ -90,11 +102,18 @@
   }
   function foo(a) { a.push(0.23441233123); }
 
+
   // 1. Optimize foo to handle fast mode arrays.
+  %PrepareFunctionForOptimization(foo);
   foo(mkArray(kFastModeLength));
   foo(mkArray(kFastModeLength));
   %OptimizeFunctionOnNextCall(foo);
   foo(mkArray(kFastModeLength));
+  assertOptimized(foo);
+
+  // Prepare foo to be re-optimized, ensuring it's bytecode / feedback vector
+  // doesn't get flushed after deoptimization.
+  %PrepareFunctionForOptimization(foo);
 
   // 2. Given a slow mode array, foo will deopt.
   foo(mkArray(kSlowModeLength));

test/mjsunit/compiler/dont-flush-code-marked-for-opt.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --opt --allow-natives-syntax --expose-gc --stress-flush-bytecode
+
+function foo(a) {}
+
+%PrepareFunctionForOptimization(foo);
+foo();
+foo();
+%OptimizeFunctionOnNextCall(foo);
+gc();
+foo();
+assertOptimized(foo);

test/mjsunit/compiler/string-from-code-point.js

@@ -10,6 +10,7 @@
     return String.fromCodePoint(x);
   }
 
+  %PrepareFunctionForOptimization(foo);
   assertEquals("\u0000", foo(0));
   assertEquals("\u0000", foo(-0));
   %OptimizeFunctionOnNextCall(foo);
@@ -17,6 +18,10 @@
   assertEquals("\u0000", foo(-0));
   assertOptimized(foo);
 
+  // Prepare foo to be re-optimized, ensuring it's bytecode / feedback vector
+  // doesn't get flushed after deoptimization.
+  %PrepareFunctionForOptimization(foo);
+
   // Now passing anything outside the valid code point
   // range should invalidate the optimized code.
   assertThrows(_ => foo(-1));

test/mjsunit/mjsunit.status

@@ -232,9 +232,6 @@
 
   # BUG(v8:8169)
   'external-backing-store-gc': [SKIP],
-
-  # https://crbug.com/v8/8781
-  'compiler/string-from-code-point': [PASS, FAIL],
 }],  # ALWAYS
 
 ['novfp3 == True', {
@@ -306,11 +303,6 @@
   # BUG(v8:4237)
   'regress/regress-3976': [SKIP],
 
-  # BUG(v8:8801): Bytecode flushing interacts poorly with assertOptimized
-  'regress/regress-7254': [SKIP],
-  'array-push5': [SKIP],
-  'compiler/deopt-array-push': [SKIP],
-
   # Slow tests.
   'array-constructor': [PASS, SLOW],
   'json': [PASS, SLOW],
@@ -421,6 +413,7 @@
   'compiler/deopt-inlined-from-call': [SKIP],
   'compiler/deopt-numberoroddball-binop': [SKIP],
   'compiler/deopt-string-outofbounds': [SKIP],
+  'compiler/dont-flush-code-marked-for-opt': [SKIP],
   'compiler/increment-typefeedback': [SKIP],
   'compiler/inlined-array-pop-opt': [SKIP],
   'compiler/inlined-call': [SKIP],

test/mjsunit/regress/regress-7254.js

@@ -9,6 +9,7 @@ function foo(a) {
   a[1] = "";
 }
 
+%PrepareFunctionForOptimization(foo);
 foo([0,0].map(x => x));
 foo([0,0].map(x => x));
 %OptimizeFunctionOnNextCall(foo);