Skip to content

Commit 1eb1520

Browse files
alexmarkovcommit-bot@chromium.org
alexmarkov
authored and
[email protected]
committed
[vm/bytecode] Prevent triggering optimizing compilation from interpreter
Previously, in rare cases it was possible that interpreter would trigger optimizing compilation (if function usage counter reached threshold in the interpreter once again after unoptimized compilation finished). This results in producing optimized code without any feedback (as unoptimized code hasn't executed). Change-Id: Ibd52b7dfe3a808ade97c02e743b40aa466b1e3c2 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108687 Commit-Queue: Alexander Markov <[email protected]> Reviewed-by: Ryan Macnak <[email protected]> Reviewed-by: Régis Crelier <[email protected]>
1 parent ed764e5 commit 1eb1520

File tree

7 files changed

+119
-97
lines changed

7 files changed

+119
-97
lines changed

‎runtime/vm/compiler/jit/compiler.cc‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -235,7 +235,7 @@ DEFINE_RUNTIME_ENTRY(CompileFunction, 1) {
235235
// If interpreter is enabled and there is bytecode, LazyCompile stub
236236
// (which calls CompileFunction) should proceed to InterpretCall in order
237237
// to enter interpreter. In such case, compilation is postponed and
238-
// triggered by interpreter later via OptimizeInvokedFunction.
238+
// triggered by interpreter later via CompileInterpretedFunction.
239239
return;
240240
}
241241
// No bytecode, fall back to compilation.
@@ -1226,13 +1226,14 @@ class BackgroundCompilationQueue {
12261226
DISALLOW_COPY_AND_ASSIGN(BackgroundCompilationQueue);
12271227
};
12281228

1229-
BackgroundCompiler::BackgroundCompiler(Isolate* isolate)
1229+
BackgroundCompiler::BackgroundCompiler(Isolate* isolate, bool optimizing)
12301230
: isolate_(isolate),
12311231
queue_monitor_(),
12321232
function_queue_(new BackgroundCompilationQueue()),
12331233
done_monitor_(),
12341234
running_(false),
12351235
done_(true),
1236+
optimizing_(optimizing),
12361237
disabled_depth_(0) {}
12371238

12381239
// Fields all deleted in ::Stop; here clear them.
@@ -1257,14 +1258,11 @@ void BackgroundCompiler::Run() {
12571258
function = function_queue()->PeekFunction();
12581259
}
12591260
while (running_ && !function.IsNull()) {
1260-
// This is false if we are compiling bytecode -> unoptimized code.
1261-
const bool optimizing = function.ShouldCompilerOptimize();
1262-
ASSERT(FLAG_enable_interpreter || optimizing);
1263-
1264-
if (optimizing) {
1261+
if (is_optimizing()) {
12651262
Compiler::CompileOptimizedFunction(thread, function,
12661263
Compiler::kNoOSRDeoptId);
12671264
} else {
1265+
ASSERT(FLAG_enable_interpreter);
12681266
Compiler::CompileFunction(thread, function);
12691267
}
12701268

@@ -1279,7 +1277,7 @@ void BackgroundCompiler::Run() {
12791277
const Function& old = Function::Handle(qelem->Function());
12801278
// If an optimizable method is not optimized, put it back on
12811279
// the background queue (unless it was passed to foreground).
1282-
if ((optimizing && !old.HasOptimizedCode() &&
1280+
if ((is_optimizing() && !old.HasOptimizedCode() &&
12831281
old.IsOptimizable()) ||
12841282
FLAG_stress_test_background_compilation) {
12851283
if (old.is_background_optimizable() &&

‎runtime/vm/compiler/jit/compiler.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ class Compiler : public AllStatic {
130130
// No OSR compilation in the background compiler.
131131
class BackgroundCompiler {
132132
public:
133-
explicit BackgroundCompiler(Isolate* isolate);
133+
explicit BackgroundCompiler(Isolate* isolate, bool optimizing);
134134
virtual ~BackgroundCompiler();
135135

136136
static void Start(Isolate* isolate) {
@@ -191,6 +191,7 @@ class BackgroundCompiler {
191191

192192
BackgroundCompilationQueue* function_queue() const { return function_queue_; }
193193
bool is_running() const { return running_; }
194+
bool is_optimizing() const { return optimizing_; }
194195

195196
void Run();
196197

@@ -210,6 +211,7 @@ class BackgroundCompiler {
210211
Monitor done_monitor_; // Notify/wait that the thread is done.
211212
bool running_; // While true, will try to read queue and compile.
212213
bool done_; // True if the thread is done.
214+
bool optimizing_;
213215

214216
int16_t disabled_depth_;
215217

‎runtime/vm/compiler/runtime_offsets_extracted.h‎

Lines changed: 55 additions & 55 deletions
Large diffs are not rendered by default.

‎runtime/vm/interpreter.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1133,11 +1133,11 @@ DART_FORCE_INLINE bool Interpreter::InstanceCall2(Thread* thread,
11331133
if (UNLIKELY(FLAG_compilation_counter_threshold >= 0 && \
11341134
counter >= FLAG_compilation_counter_threshold && \
11351135
!Function::HasCode(function))) { \
1136-
SP[1] = 0; /* Unused code result. */ \
1136+
SP[1] = 0; /* Unused result. */ \
11371137
SP[2] = function; \
11381138
Exit(thread, FP, SP + 3, pc); \
11391139
NativeArguments native_args(thread, 1, SP + 2, SP + 1); \
1140-
INVOKE_RUNTIME(DRT_OptimizeInvokedFunction, native_args); \
1140+
INVOKE_RUNTIME(DRT_CompileInterpretedFunction, native_args); \
11411141
function = FrameFunction(FP); \
11421142
} \
11431143
}
@@ -1766,11 +1766,11 @@ RawObject* Interpreter::Call(RawFunction* function,
17661766
if (UNLIKELY(FLAG_compilation_counter_threshold >= 0 &&
17671767
counter >= FLAG_compilation_counter_threshold &&
17681768
!Function::HasCode(function))) {
1769-
SP[1] = 0; // Unused code result.
1769+
SP[1] = 0; // Unused result.
17701770
SP[2] = function;
17711771
Exit(thread, FP, SP + 3, pc);
17721772
NativeArguments native_args(thread, 1, SP + 2, SP + 1);
1773-
INVOKE_RUNTIME(DRT_OptimizeInvokedFunction, native_args);
1773+
INVOKE_RUNTIME(DRT_CompileInterpretedFunction, native_args);
17741774
}
17751775
DISPATCH();
17761776
}

‎runtime/vm/isolate.cc‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -963,10 +963,11 @@ Isolate::Isolate(IsolateGroup* isolate_group,
963963
}
964964

965965
if (FLAG_enable_interpreter) {
966-
NOT_IN_PRECOMPILED(background_compiler_ = new BackgroundCompiler(this));
966+
NOT_IN_PRECOMPILED(background_compiler_ = new BackgroundCompiler(
967+
this, /* optimizing = */ false));
967968
}
968969
NOT_IN_PRECOMPILED(optimizing_background_compiler_ =
969-
new BackgroundCompiler(this));
970+
new BackgroundCompiler(this, /* optimizing = */ true));
970971

971972
isolate_group->RegisterIsolate(this);
972973
isolate_group_ = isolate_group;

‎runtime/vm/runtime_entry.cc‎

Lines changed: 47 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2292,28 +2292,56 @@ DEFINE_RUNTIME_ENTRY(TraceICCall, 2) {
22922292
ic_data.NumberOfChecks(), function.ToFullyQualifiedCString());
22932293
}
22942294

2295+
// This is called from interpreter when function usage counter reached
2296+
// compilation threshold and function needs to be compiled.
2297+
DEFINE_RUNTIME_ENTRY(CompileInterpretedFunction, 1) {
2298+
#if !defined(DART_PRECOMPILED_RUNTIME)
2299+
const Function& function = Function::CheckedHandle(zone, arguments.ArgAt(0));
2300+
ASSERT(!function.IsNull());
2301+
ASSERT(FLAG_enable_interpreter);
2302+
2303+
#if !defined(PRODUCT)
2304+
if (Debugger::IsDebugging(thread, function)) {
2305+
return;
2306+
}
2307+
#endif // !defined(PRODUCT)
2308+
2309+
if (FLAG_background_compilation) {
2310+
if (!BackgroundCompiler::IsDisabled(isolate,
2311+
/* optimizing_compilation = */ false) &&
2312+
function.is_background_optimizable()) {
2313+
// Ensure background compiler is running, if not start it.
2314+
BackgroundCompiler::Start(isolate);
2315+
// Reduce the chance of triggering a compilation while the function is
2316+
// being compiled in the background. INT_MIN should ensure that it
2317+
// takes long time to trigger a compilation.
2318+
// Note that the background compilation queue rejects duplicate entries.
2319+
function.SetUsageCounter(INT_MIN);
2320+
isolate->background_compiler()->Compile(function);
2321+
return;
2322+
}
2323+
}
2324+
2325+
// Reset usage counter for future optimization.
2326+
function.SetUsageCounter(0);
2327+
Object& result =
2328+
Object::Handle(zone, Compiler::CompileFunction(thread, function));
2329+
ThrowIfError(result);
2330+
#else
2331+
UNREACHABLE();
2332+
#endif // !DART_PRECOMPILED_RUNTIME
2333+
}
2334+
22952335
// This is called from function that needs to be optimized.
22962336
// The requesting function can be already optimized (reoptimization).
22972337
// Returns the Code object where to continue execution.
22982338
DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
22992339
#if !defined(DART_PRECOMPILED_RUNTIME)
23002340
const Function& function = Function::CheckedHandle(zone, arguments.ArgAt(0));
23012341
ASSERT(!function.IsNull());
2342+
ASSERT(function.HasCode());
23022343

2303-
// If running with interpreter, do the unoptimized compilation first.
2304-
const bool optimizing_compilation = function.ShouldCompilerOptimize();
2305-
ASSERT(FLAG_enable_interpreter || optimizing_compilation);
2306-
ASSERT((!optimizing_compilation) || function.HasCode() ||
2307-
function.ForceOptimize());
2308-
2309-
#if defined(PRODUCT)
2310-
if (!optimizing_compilation ||
2311-
Compiler::CanOptimizeFunction(thread, function)) {
2312-
#else
2313-
if ((!optimizing_compilation && !Debugger::IsDebugging(thread, function)) ||
2314-
(optimizing_compilation &&
2315-
Compiler::CanOptimizeFunction(thread, function))) {
2316-
#endif // defined(PRODUCT)
2344+
if (Compiler::CanOptimizeFunction(thread, function)) {
23172345
if (FLAG_background_compilation) {
23182346
if (FLAG_enable_inlining_annotations) {
23192347
FATAL("Cannot enable inlining annotations and background compilation");
@@ -2328,7 +2356,8 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
23282356
// Get next field.
23292357
field = isolate->GetDeoptimizingBoxedField();
23302358
}
2331-
if (!BackgroundCompiler::IsDisabled(isolate, optimizing_compilation) &&
2359+
if (!BackgroundCompiler::IsDisabled(isolate,
2360+
/* optimizing_compiler = */ true) &&
23322361
function.is_background_optimizable()) {
23332362
// Ensure background compiler is running, if not start it.
23342363
BackgroundCompiler::Start(isolate);
@@ -2337,12 +2366,7 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
23372366
// takes long time to trigger a compilation.
23382367
// Note that the background compilation queue rejects duplicate entries.
23392368
function.SetUsageCounter(INT_MIN);
2340-
if (optimizing_compilation) {
2341-
isolate->optimizing_background_compiler()->Compile(function);
2342-
} else {
2343-
ASSERT(FLAG_enable_interpreter);
2344-
isolate->background_compiler()->Compile(function);
2345-
}
2369+
isolate->optimizing_background_compiler()->Compile(function);
23462370
// Continue in the same code.
23472371
arguments.SetReturn(function);
23482372
return;
@@ -2358,12 +2382,8 @@ DEFINE_RUNTIME_ENTRY(OptimizeInvokedFunction, 1) {
23582382
function.ToFullyQualifiedCString());
23592383
}
23602384
}
2361-
Object& result = Object::Handle(zone);
2362-
if (optimizing_compilation) {
2363-
result = Compiler::CompileOptimizedFunction(thread, function);
2364-
} else {
2365-
result = Compiler::CompileFunction(thread, function);
2366-
}
2385+
Object& result = Object::Handle(
2386+
zone, Compiler::CompileOptimizedFunction(thread, function));
23672387
ThrowIfError(result);
23682388
}
23692389
arguments.SetReturn(function);

‎runtime/vm/runtime_entry_list.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ namespace dart {
5454
V(UpdateFieldCid) \
5555
V(InitStaticField) \
5656
V(CompileFunction) \
57+
V(CompileInterpretedFunction) \
5758
V(MonomorphicMiss) \
5859
V(SingleTargetMiss) \
5960
V(UnlinkedCall)

0 commit comments

Comments
 (0)