On Mon, 12 Jun 2023 12:00:48 GMT, Andrei Pangin <apan...@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
