On Thu, 14 Dec 2023 15:29:06 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 (so bypass inlined 
>> methods).
>> 
>> Natural way to eliminate the discrepancy between the result of compilation 
>> and the broken rule is to discard the compilation result, i.e. 
>> deoptimization. Prior to that we can try to re-compile the method letting 
>> compile broker to perform it taking new directives stack into account. 
>> Re-compilation helps to prevent hot methods from execution in the 
>> interpreter.
>> 
>> A new flag `-r` 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 
>> puts methods that have any active non-default matching compiler directives 
>> to re-compilation if possible, otherwise marks them for deoptimization. 
>> 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 
>> refreshed. On the other hand, if there are rules for a method and it was 
>> inlined, top-level methods won't be refreshed, but this can be achieved by 
>> having rules for them.
>> 
>> In addition, a new diagnostic command `Compiler.replace_directives...
>
> Dmitry Chuyko has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 33 commits:
> 
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>  - ... and 23 more: https://git.openjdk.org/jdk/compare/fde5b168...44d680cd

src/hotspot/share/code/codeCache.cpp line 1409:

> 1407:   while(iter.next()) {
> 1408:     CompiledMethod* nm = iter.method();
> 1409:     methodHandle mh(thread, nm->method());

If there are two CompiledMethods for the same Java method, will it be scheduled 
for recompilation twice? Related question: if `nm` is an OSR method, does it 
make sense to go directly for deoptimization rather than compiling a non-OSR 
version?

src/hotspot/share/code/codeCache.cpp line 1413:

> 1411:       ResourceMark rm;
> 1412:       // Try the max level and let the directives be applied during the 
> compilation.
> 1413:       int complevel = CompLevel::CompLevel_full_optimization;

Should the highest level depend on the configuration instead of the hard-coded 
constant? Perhaps, needs to be `highest_compile_level()`

src/hotspot/share/compiler/compilerDirectives.cpp line 750:

> 748:       if (!dir->is_default_directive() && dir->match(method)) {
> 749:         match_found = true;
> 750:         break;

`match_found` is redundant: for better readability, you may just return true. 
Curly braces around MutexLocker won't be needed either.

src/hotspot/share/oops/method.hpp line 820:

> 818:   // Clear the flags related to compiler directives that were set by the 
> compilerBroker,
> 819:   // because the directives can be updated.
> 820:   void clear_method_flags() {

The function name is a bit misleading - it clears only flags related to 
directives.

src/hotspot/share/oops/methodFlags.hpp line 61:

> 59:    status(has_loops_flag_init         , 1 << 14) /* The loop flag has 
> been initialized */ \
> 60:    status(on_stack_flag               , 1 << 15) /* RedefineClasses 
> support to keep Metadata from being cleaned */ \
> 61:    status(has_matching_directives     , 1 << 16) /* The method has 
> matching directives */ \

It's worth noting that the flag is temporary and is valid only during DCmd 
execution.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1432195677
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1432187571
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1432200716
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1432210229
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1432212171

Reply via email to