On Wed, 24 May 2023 00:38:27 GMT, Dmitry Chuyko <dchu...@openjdk.org> wrote:
> Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives. Methods in general are > often inlined, and this information is hard to track down. > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Obviously there is a performance penalty, so it should be > applied with care. Hot code will most likely be recompiled soon, as nothing > happens to its hotness. > > A new flag '`-d`' has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > marks for deoptimization those methods that have any active non-default > matching compiler directives. There is currently no distinction which > directives are found. In particular, this means that if there are rules for > inlining into some method, it will be deoptimized. On the other hand, if > there are rules for a method and it was inlined, top-level methods won't be > deoptimized, but this can be achieved by having rules for them. > > In addition, a new diagnistic command `Compiler.replace_directives`, has been > added for convenience. It's like a combinatio... src/hotspot/share/code/codeCache.cpp line 1379: > 1377: while(iter.next()) { > 1378: CompiledMethod* nm = iter.method(); > 1379: HandleMark hm(thread); Isn't it better to move `HandleMark` outside the loop? src/hotspot/share/runtime/mutexLocker.cpp line 274: > 272: MUTEX_DEFN(MethodCompileQueue_lock , PaddedMonitor, safepoint); > 273: MUTEX_DEFN(CompileStatistics_lock , PaddedMutex , safepoint); > 274: MUTEX_DEFN(DirectivesStack_lock , PaddedMutex , > nosafepoint-3); A comment explaining the rank change would be helpful. src/hotspot/share/services/diagnosticCommand.cpp line 890: > 888: DCmdWithParser(output, heap), > 889: _filename("filename", "Name of the directives file", "STRING", true), > 890: _force_deopt("-d", "Force deoptimization of affected methods.", > "BOOLEAN", false, "false") { I agree with Paul a generic alternative like `refresh` would be better. src/hotspot/share/services/diagnosticCommand.cpp line 946: > 944: DeoptimizationScope deopt_scope; > 945: CodeCache::mark_for_deoptimization_directives_matches(&deopt_scope); > 946: DirectivesStack::pop(1); Why deoptimizing methods from the whole stack if we change only the topmost list? src/hotspot/share/services/diagnosticCommand.hpp line 734: > 732: }; > 733: > 734: class CompilerDirectivesReplaceDCmd : public DCmdWithParser { Introducing a new command probably isn't worth it, given the same functionality can be achieved with existing commands. Furthermore, it's not obvious whether "replace" should mean remove+add or clear+add. src/hotspot/share/services/diagnosticCommand.hpp line 745: > 743: } > 744: static const char* description() { > 745: return "Clear derectives stack amd load new compiler directives from > file."; Spelling: Clear *directives* stack *and* ... ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226618742 PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226553192 PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226624671 PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226627598 PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226604676 PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1226548048
