RFR: 8300926: Several startup regressions ~6-70% in 21-b6 all platforms [v2]

[Daniel D. Daugherty]

On Fri, 17 Feb 2023 09:25:55 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi all, please consider.
>> 
>> The original issue was when thread 1 asked to deopt nmethod set X and thread 2 asked for the same or a subset of X.
>> All method will already be marked, but the actual deoptimizing, not entrant, patching PC on stacks and patching post call nops, was not done yet. Which meant thread 2 could 'pass' thread 1.
>> Most places did deopt under Compile_lock, thus this is not an issue, but WB and clearCallSiteContext do not.
>> 
>> Since a handshakes may take long before completion and Compile_lock is used for so much more than deopts.
>> The fix in https://bugs.openjdk.org/browse/JDK-8299074 instead always emitted a handshake even when everything was already marked. (instead of adding Compile_lock to all places)
>> 
>> This turnout to be problematic in the startup, for example the number of deopt handshakes in jetty dry run startup went from 5 to 39 handshakes.
>> 
>> This fix first adds a barrier for which you do not pass until the requested deopts have happened and it coalesces the handshakes.
>> Secondly it moves handshakes part out of the Compile_lock where it is possible.
>> 
>> Which means we fix the performance bug and we reduce the contention on Compile_lock, meaning higher throughput in compiler and things such as class-loading.
>> 
>> It passes t1-t7 with flying colours! t8 still not completed and I'm redoing some testing due to last minute simplifications.
>> 
>> Thanks, Robbin
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review fixes

Another round of editorial changes.

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

> 1414: }
> 1415: 
> 1416: // Flushes compiled methods dependent on dependee

nit typo: s/Flushes/Marks/

src/hotspot/share/code/codeCache.hpp line 316:

> 314: 
> 315:   // RedefineClasses support
> 316:   // Flushing and deoptimization in case of evolution

nit typo: s/Flushing/Marking/

src/hotspot/share/oops/instanceKlass.cpp line 1184:

> 1182:     DeoptimizationScope deopt_scope;
> 1183:     {
> 1184:       // Now flush all code that assume the class is not linked.

nit typo: s/assume/assumes/

src/hotspot/share/prims/methodHandles.cpp line 959:

> 957:   {
> 958:     NoSafepointVerifier nsv;
> 959:     MutexLocker mu2(CodeCache_lock, Mutex::_no_safepoint_check_flag);

I missed this naming issue earlier: s/mu2/mu/
since we only have one Mutex here.

src/hotspot/share/prims/methodHandles.cpp line 1222:

> 1220:     MethodHandles::mark_dependent_nmethods(&deopt_scope, call_site, target);
> 1221:     java_lang_invoke_CallSite::set_target(call_site(), target());
> 1222:     // This assumed to be an 'atomic' operation by verification.

nit typo: s/This assumed/This is assumed/

src/hotspot/share/prims/methodHandles.cpp line 1238:

> 1236:     MethodHandles::mark_dependent_nmethods(&deopt_scope, call_site, target);
> 1237:     java_lang_invoke_CallSite::set_target_volatile(call_site(), target());
> 1238:     // This assumed to be an 'atomic' operation by verification.

nit typo: s/This assumed/This is assumed/

src/hotspot/share/prims/methodHandles.cpp line 1336:

> 1334:     DependencyContext deps = java_lang_invoke_MethodHandleNatives_CallSiteContext::vmdependencies(context());
> 1335:     deps.remove_and_mark_for_deoptimization_all_dependents(&deopt_scope);
> 1336:     // This assumed to be an 'atomic' operation by verification.

nit typo: s/This assumed/This is assumed/

src/hotspot/share/prims/methodHandles.hpp line 82:

> 80:   static void clean_dependency_context(oop call_site);
> 81: 
> 82:   static void mark_dependent_nmethods(DeoptimizationScope* deopt_scope, Handle call_site, Handle target);

Need to update copyright year in this file.

src/hotspot/share/runtime/deoptimization.cpp line 109:

> 107: 
> 108:   MutexLocker ml(CompiledMethod_lock, Mutex::_no_safepoint_check_flag);
> 109:   // If there is nothing to deopt _gen is the same as comitted.

nit typo: s/_gen/_required_gen/

src/hotspot/share/runtime/deoptimization.cpp line 143:

> 141:                  Mutex::_no_safepoint_check_flag);
> 142:   // A method marked by someone else may have a _gen lower than what we marked with.
> 143:   // Therefore only store it if it's higher than _gen.

nit typo: s/_gen/_required_gen/
in two places

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

PR: https://git.openjdk.org/jdk/pull/12585