Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v15]
On Wed, 20 Dec 2023 02:57:29 GMT, Andrei Pangin wrote:
>> 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?
If there are multiple method versions, it will be recompiled several times. The
alternative is too keep some additional information which may complicate the
code. OSRs is a good catch, I changed their handling to deopt.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1434887812
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v15]
On Wed, 20 Dec 2023 02:40:40 GMT, Andrei Pangin wrote:
>> 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 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()`
Yes, changed to use `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.
Thanks, that's indeed simpler.
> 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.
Changed to `clear_directive_flags`.
> 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.
Good point, updated the comment. This btw means that in another places this
flag can be reused.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1434883459
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1434884163
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1434884612
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1434885291
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v15]
On Thu, 14 Dec 2023 15:29:06 GMT, Dmitry Chuyko 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
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v15]
> 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`, has been > added for ... 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 - Changes: https://git.openjdk.org/jdk/pull/14111/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14111=14 Stats: 372 lines in 15 files changed: 339 ins; 3 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/14111.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14111/head:pull/14111 PR: https://git.openjdk.org/jdk/pull/14111
