RFR: 8309271: A way to align already compiled methods with compiler directives

[Andrei Pangin]

On Wed, 24 May 2023 00:38:27 GMT, Dmitry Chuyko <dchuyko at 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