On Mon, 13 May 2024 21:08:08 GMT, Evgeny Astigeevich <eastigeev...@openjdk.org> wrote:
>> Backout of [JDK-8309271](https://bugs.openjdk.org/browse/JDK-8309271) which >> has known bugs, possible bugs and performance issues. REDO work is tracked >> by [JDK-8331749](https://bugs.openjdk.org/browse/JDK-8331749). >> >> Found bugs: >> - When refreshing `CompilerDirectivesAddDCmd::execute` will call >> `DirectivesStack::hasMatchingDirectives(mh, true)` which only considers the >> compiler directive which is on the top of the directives stack. As more than >> one directive can be added, `CompilerDirectivesAddDCmd::execute` will not >> behave as expected. >> - A Java method with old directives might be in the compilation queue. A >> request to recompile it with new directives will be ignored. >> >> There are other concerns: bugs and performance issues. >> >> Possible bugs: >> - `has_matching_directives` might not be cleared. A nmethod might get into >> the unloading state before `CodeCache::recompile_marked_directives_matches`. >> If the nmethod has been used to mark a Java method and it is the only >> nmethod, there will be no nmethod in CodeCache to reach the Java method to >> clear the mark. >> - A Java method might have been compiled with new directives before >> `CodeCache::recompile_marked_directives_matches`. >> `CodeCache::recompile_marked_directives_matches` will recompile it again. >> - JIT compiler might be compiling a Java method with old directives. A >> request to recompile it with new directives will be ignored. >> >> Performance issues: >> - Usually directives are updated for a small number of Java methods. If >> CodeCache has thousands of nmethods, >> `CodeCache::recompile_marked_directives_matches` will be traversing nmethods >> most of which don't need recompilation. >> >> The backout is not clean because of removal of `CompiledMethod`. >> >> Tested with release and fastdebug builds: tier1 and tier2 passed. > > What if instead of backing out we will use an experimental JVM flag: > `XX:+CompilerDirectivesRefreshSupport`? > I agree with this backout. Thank you @eastig for explaining your point. We > have about 3 weeks before RDP1 and it is better we have less issues before > that. Let redo implementation in next release taking into account the issues > you found and have more time for testing. OK. I hope it takes less time to get back into the source tree than it did initially. ------------- PR Comment: https://git.openjdk.org/jdk/pull/19215#issuecomment-2109874596
