From d698ef52ab913384473c39687c32d68592422132 Mon Sep 17 00:00:00 2001 From: Alex Light Date: Mon, 2 Apr 2018 11:28:50 -0700 Subject: Revert^6 "Ensure that OSR still is possible with jvmti" The instrumentation uninstall could set methods to non-debuggable boot.oat code. This could cause events to be missed due to methods being inlined. We needed to change the path so that we would only have the JIT/interpreter replace methods. We do this by adding a new callback that can be used to determine if a method needs to be debuggable and being more careful about replacing code when this is true. This reverts commit 5f3005c8844d851d7d218b88b5f90d6c9083ce24. This unreverts commit b9ad26d1ed9146b89555d4333021f44eeb831f05. Reason for revert: Fixed issue causing CTS version of test 993 failure. Test: cts-tradefed run cts-dev CtsJvmtiRunTest993HostTestCases Test: ./test.py --host -j50 --all -t 993 Test: ./test.py --host Test: while ./test/run-test --host --jit 1935; do; done Test: while ./test/run-test --host --jit --jvmti-redefine-stress 1935; do; done Test: am start --attach-agent -n com.example.android.displayingbitmaps/.ui.ImageGridActivity Run blur filter. Bug: 76226464 Bug: 77306669 Merged-In: I5068201a03f7613787c66981405499b6499c24e1 Change-Id: I5068201a03f7613787c66981405499b6499c24e1 (cherry picked from commit f28586390b055a5681e50617d729a3fa09792d9c) --- openjdkjvmti/deopt_manager.cc | 79 +++++++++++++++++++++++++++---------------- 1 file changed, 49 insertions(+), 30 deletions(-) (limited to 'openjdkjvmti/deopt_manager.cc') diff --git a/openjdkjvmti/deopt_manager.cc b/openjdkjvmti/deopt_manager.cc index 6d84ffa53f..a6f1207f34 100644 --- a/openjdkjvmti/deopt_manager.cc +++ b/openjdkjvmti/deopt_manager.cc @@ -53,20 +53,29 @@ namespace openjdkjvmti { // TODO We should make this much more selective in the future so we only return true when we -// actually care about the method (i.e. had locals changed, have breakpoints, etc.). For now though -// we can just assume that we care we are loaded at all. -// -// Even if we don't keep track of this at the method level we might want to keep track of it at the -// level of enabled capabilities. -bool JvmtiMethodInspectionCallback::IsMethodBeingInspected( - art::ArtMethod* method ATTRIBUTE_UNUSED) { - return true; +// actually care about the method at this time (ie active frames had locals changed). For now we +// just assume that if anything has changed any frame's locals we care about all methods. If nothing +// has we only care about methods with active breakpoints on them. In the future we should probably +// rewrite all of this to instead do this at the ShadowFrame or thread granularity. +bool JvmtiMethodInspectionCallback::IsMethodBeingInspected(art::ArtMethod* method) { + // Non-java-debuggable runtimes we need to assume that any method might not be debuggable and + // therefore potentially being inspected (due to inlines). If we are debuggable we rely hard on + // inlining not being done since we don't keep track of which methods get inlined where and simply + // look to see if the method is breakpointed. + return !art::Runtime::Current()->IsJavaDebuggable() || + manager_->HaveLocalsChanged() || + manager_->MethodHasBreakpoints(method); } bool JvmtiMethodInspectionCallback::IsMethodSafeToJit(art::ArtMethod* method) { return !manager_->MethodHasBreakpoints(method); } +bool JvmtiMethodInspectionCallback::MethodNeedsDebugVersion( + art::ArtMethod* method ATTRIBUTE_UNUSED) { + return true; +} + DeoptManager::DeoptManager() : deoptimization_status_lock_("JVMTI_DeoptimizationStatusLock", static_cast( @@ -75,7 +84,10 @@ DeoptManager::DeoptManager() performing_deoptimization_(false), global_deopt_count_(0), deopter_count_(0), - inspection_callback_(this) { } + breakpoint_status_lock_("JVMTI_BreakpointStatusLock", + static_cast(art::LockLevel::kAbortLock + 1)), + inspection_callback_(this), + set_local_variable_called_(false) { } void DeoptManager::Setup() { art::ScopedThreadStateChange stsc(art::Thread::Current(), @@ -121,14 +133,11 @@ void DeoptManager::FinishSetup() { } bool DeoptManager::MethodHasBreakpoints(art::ArtMethod* method) { - art::MutexLock lk(art::Thread::Current(), deoptimization_status_lock_); + art::MutexLock lk(art::Thread::Current(), breakpoint_status_lock_); return MethodHasBreakpointsLocked(method); } bool DeoptManager::MethodHasBreakpointsLocked(art::ArtMethod* method) { - if (deopter_count_ == 0) { - return false; - } auto elem = breakpoint_status_.find(method); return elem != breakpoint_status_.end() && elem->second != 0; } @@ -158,18 +167,23 @@ void DeoptManager::AddMethodBreakpoint(art::ArtMethod* method) { art::ScopedThreadSuspension sts(self, art::kSuspended); deoptimization_status_lock_.ExclusiveLock(self); - - DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request"; - - if (MethodHasBreakpointsLocked(method)) { - // Don't need to do anything extra. - breakpoint_status_[method]++; - // Another thread might be deoptimizing the very method we just added new breakpoints for. Wait - // for any deopts to finish before moving on. - WaitForDeoptimizationToFinish(self); - return; + { + breakpoint_status_lock_.ExclusiveLock(self); + + DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request"; + + if (MethodHasBreakpointsLocked(method)) { + // Don't need to do anything extra. + breakpoint_status_[method]++; + // Another thread might be deoptimizing the very method we just added new breakpoints for. + // Wait for any deopts to finish before moving on. + breakpoint_status_lock_.ExclusiveUnlock(self); + WaitForDeoptimizationToFinish(self); + return; + } + breakpoint_status_[method] = 1; + breakpoint_status_lock_.ExclusiveUnlock(self); } - breakpoint_status_[method] = 1; auto instrumentation = art::Runtime::Current()->GetInstrumentation(); if (instrumentation->IsForcedInterpretOnly()) { // We are already interpreting everything so no need to do anything. @@ -196,17 +210,22 @@ void DeoptManager::RemoveMethodBreakpoint(art::ArtMethod* method) { // need but since that is very heavy we will instead just use a condition variable to make sure we // don't race with ourselves. deoptimization_status_lock_.ExclusiveLock(self); - - DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request"; - DCHECK(MethodHasBreakpointsLocked(method)) << "Breakpoint on a method was removed without " - << "breakpoints present!"; + bool is_last_breakpoint; + { + art::MutexLock mu(self, breakpoint_status_lock_); + + DCHECK_GT(deopter_count_, 0u) << "unexpected deotpimization request"; + DCHECK(MethodHasBreakpointsLocked(method)) << "Breakpoint on a method was removed without " + << "breakpoints present!"; + breakpoint_status_[method] -= 1; + is_last_breakpoint = (breakpoint_status_[method] == 0); + } auto instrumentation = art::Runtime::Current()->GetInstrumentation(); - breakpoint_status_[method] -= 1; if (UNLIKELY(instrumentation->IsForcedInterpretOnly())) { // We don't need to do anything since we are interpreting everything anyway. deoptimization_status_lock_.ExclusiveUnlock(self); return; - } else if (breakpoint_status_[method] == 0) { + } else if (is_last_breakpoint) { if (UNLIKELY(is_default)) { RemoveDeoptimizeAllMethodsLocked(self); } else { -- cgit v1.2.3-59-g8ed1b