ART: Fix race in on-stack replacement
The expected sequence of events for on-stack replacement is:
1. Method goes warm, triggering enhanced profiling
2. Method goes hot, triggering method compilation
3. Method goes really hot, triggering an osr method compilation.
4. Interpreter polls for the existence of an osr entry point,
and transitons to compiled code if found.
We have a race problem if #2 and #3 happen closely together, and
the osr method compilation begins before the regular method
compilation. In that case, the jit sees that the method is
already being compiled (the osr method - but it does not
distinguish the two), and discards the normal compilation request.
So, the osr version is compiled and the normal version is discarded.
In #4, the MaybeDoOnStackReplacement() check assumes that a normal
version of the compiled method must exist before doing an on-stack
replacement, so it keeps returning false.
This is why we were seeing sporadic timeout failures of
570-checker-osr when the mterp fast branch profiling was
introduced. The branch profiling performance enhancements
greatly reduced the time between #2 and #3, increasing the liklihood
of losing the race. Further, the new code clamped hotness to avoid
wrap-around. The race existed (and likely occurred) in the previous
version, but because hotness counters were allowed to overflow and
wrap around you'd eventually hit the threshold a second time and
try again - masking the problem.
Tip 'o the hat to Serguei Katkov for identifying the problem.
A possible solution (taken in this CL) is to differentiate osr
compilations from normal method compilations.
Bug: 27939339
Change-Id: I71044516b35dc69de9fc2d2a445e33809ac650ed
diff --git a/runtime/jit/jit.cc b/runtime/jit/jit.cc
index 73aaf04..0254727 100644
--- a/runtime/jit/jit.cc
+++ b/runtime/jit/jit.cc
@@ -208,7 +208,7 @@
return false;
}
bool success = jit_compile_method_(jit_compiler_handle_, method_to_compile, self, osr);
- code_cache_->DoneCompiling(method_to_compile, self);
+ code_cache_->DoneCompiling(method_to_compile, self, osr);
return success;
}
diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc
index 37ff6a5..820ae6a 100644
--- a/runtime/jit/jit_code_cache.cc
+++ b/runtime/jit/jit_code_cache.cc
@@ -927,12 +927,12 @@
return false;
}
- if (info->IsMethodBeingCompiled()) {
+ if (info->IsMethodBeingCompiled(osr)) {
VLOG(jit) << PrettyMethod(method) << " is already being compiled";
return false;
}
- info->SetIsMethodBeingCompiled(true);
+ info->SetIsMethodBeingCompiled(true, osr);
return true;
}
@@ -952,10 +952,10 @@
info->DecrementInlineUse();
}
-void JitCodeCache::DoneCompiling(ArtMethod* method, Thread* self ATTRIBUTE_UNUSED) {
+void JitCodeCache::DoneCompiling(ArtMethod* method, Thread* self ATTRIBUTE_UNUSED, bool osr) {
ProfilingInfo* info = method->GetProfilingInfo(sizeof(void*));
- DCHECK(info->IsMethodBeingCompiled());
- info->SetIsMethodBeingCompiled(false);
+ DCHECK(info->IsMethodBeingCompiled(osr));
+ info->SetIsMethodBeingCompiled(false, osr);
}
size_t JitCodeCache::GetMemorySizeOfCodePointer(const void* ptr) {
diff --git a/runtime/jit/jit_code_cache.h b/runtime/jit/jit_code_cache.h
index 6faa8f1..9f18c70 100644
--- a/runtime/jit/jit_code_cache.h
+++ b/runtime/jit/jit_code_cache.h
@@ -80,7 +80,7 @@
SHARED_REQUIRES(Locks::mutator_lock_)
REQUIRES(!lock_);
- void DoneCompiling(ArtMethod* method, Thread* self)
+ void DoneCompiling(ArtMethod* method, Thread* self, bool osr)
SHARED_REQUIRES(Locks::mutator_lock_)
REQUIRES(!lock_);
diff --git a/runtime/jit/profiling_info.h b/runtime/jit/profiling_info.h
index 55d627a..3a71bba 100644
--- a/runtime/jit/profiling_info.h
+++ b/runtime/jit/profiling_info.h
@@ -119,12 +119,18 @@
InlineCache* GetInlineCache(uint32_t dex_pc);
- bool IsMethodBeingCompiled() const {
- return is_method_being_compiled_;
+ bool IsMethodBeingCompiled(bool osr) const {
+ return osr
+ ? is_osr_method_being_compiled_
+ : is_method_being_compiled_;
}
- void SetIsMethodBeingCompiled(bool value) {
- is_method_being_compiled_ = value;
+ void SetIsMethodBeingCompiled(bool value, bool osr) {
+ if (osr) {
+ is_osr_method_being_compiled_ = value;
+ } else {
+ is_method_being_compiled_ = value;
+ }
}
void SetSavedEntryPoint(const void* entry_point) {
@@ -155,7 +161,8 @@
}
bool IsInUseByCompiler() const {
- return IsMethodBeingCompiled() || (current_inline_uses_ > 0);
+ return IsMethodBeingCompiled(/*osr*/ true) || IsMethodBeingCompiled(/*osr*/ false) ||
+ (current_inline_uses_ > 0);
}
private:
@@ -181,6 +188,7 @@
// is implicitly guarded by the JIT code cache lock.
// TODO: Make the JIT code cache lock global.
bool is_method_being_compiled_;
+ bool is_osr_method_being_compiled_;
// When the compiler inlines the method associated to this ProfilingInfo,
// it updates this counter so that the GC does not try to clear the inline caches.