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

[Dmitry Chuyko]

On Mon, 12 Jun 2023 12:00:48 GMT, Andrei Pangin <apangin at openjdk.org> 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 23 commits:
>> 
>>  - jcheck
>>  - Unnecessary import
>>  - force_update->refresh
>>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>>  - Use only top directive for add/remove; better mutex rank definition; texts
>>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>>  - Safe handling of non-java methods
>>  - Merge branch 'openjdk:master' into compiler-directives-force-update
>>  - Renamed -d to -r, clear not compilable flags when directives change, original test restored, new test added
>>  - ... and 13 more: https://git.openjdk.org/jdk/compare/de0e46c2...e361b4fc
>
> 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.

Changed the definition to use another macro and relative ranking.

> 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.

Changed the flag, description and names to 'refresh', 'r'.

> 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?

Agree, now only top directive is involved for add/remove.

> 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.

'replace' is 'clear'+'add' (we don't have indexes and don't mention top element). Additional command is made to avoid double refreshing when '-r' is provided. It is more robust as we don't know when methods affected 'clear' are finally compiled.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1304026590
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1304088822
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1304028173
PR Review Comment: https://git.openjdk.org/jdk/pull/14111#discussion_r1302901891