RFR: 8299074: nmethod marked for deoptimization is not deoptimized [v2]

[Robbin Ehn]

On Tue, 17 Jan 2023 15:30:46 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> In the failing `UnexpectedDeoptimizationAllTest` there is a race condition between one thread repeatedly calling `WB_DeoptimizeAll()` and the main thread checking nmethod dependencies on class loading and also attempting marking/deoptimization of nmethods due to dependency violations. The problem is that already (concurrently) marked nmethods are not counted and if no new nmethods are marked, we then wrongly assume that there is no need to call `deoptimize_all_marked`:
>> https://github.com/openjdk/jdk/blob/08008139cc05a8271e7163eca47d2bc59db2049b/src/hotspot/share/code/codeCache.cpp#L1438-L1440
>> 
>> Details:
>> 
>> **Thread 1:** Method `useNativeAccessor(Executable member)` is compiled under the assumption that its argument type `java.lang.reflect.Executable` has only one implementer `java.lang.reflect.Method`. A corresponding dependency is registered in the nmethod and a virtual call to `member.getParameterCount()` is inlined to `java.lang.reflect.Method::getParameterCount()`.
>> 
>> **Thread 2:** Calls Whitebox API method `WB_DeoptimizeAll` -> `CodeCache::mark_all_nmethods_for_deoptimization()` that marks `useNativeAccessor` for deoptimization.
>> 
>> **Thread 1:** Triggers class loading of `java.lang.reflect.Constructor` and `CodeCache::flush_dependents_on` -> `CodeCache::mark_for_deoptimization` -> ... -> `DependencyContext::mark_dependent_nmethods` detects that `useNativeAccessor` needs to be deoptimized now that `java.lang.reflect.Executable` has more than one implementer. However, the nmethod is already marked for deoptimization (most nmethods are) and therefore ignored. The marked counter is 0 and therefore `Deoptimization::deoptimize_all_marked()` is not executed either. The thread continues execution and ends up crashing because a `java.lang.reflect.Constructor` object is passed to compiled `useNativeAccessor` which can not handle it (we call the wrong `getParameterCount` on it).
>> 
>> **Thread 2:** Is still in `WB_DeoptimizeAll`, marking nmethods for deoptimization but didn't get a chance to call `Deoptimization::deoptimize_all_marked()` yet.
>> 
>> Before [JDK-8221734](https://bugs.openjdk.org/browse/JDK-8221734) in JDK 13, `WB_DeoptimizeAll` acquired the `Compile_lock` but it got [removed](http://hg.openjdk.java.net/jdk/jdk/rev/9b70ebd131b4#l15.7). Instead of re-adding the lock, I modified the code to also count nmethods that were already marked for deoptimization, as suggested by @robehn in the bug comments.
>> 
>> Thanks,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Do not check the dependency on already marked nmethods

Marked as reviewed by rehn (Reviewer).

src/hotspot/share/code/dependencyContext.cpp line 86:

> 84:         changes.mark_for_deoptimization(nm);
> 85:         found++;
> 86:       }

A minor suggestion is to move the found++ from the individual branches to here, so there is only one.
Looks good!

-------------

PR: https://git.openjdk.org/jdk/pull/12012