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

[Robbin Ehn]

On Thu, 16 Feb 2023 20:17:29 GMT, Daniel D. Daugherty <dcubed 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
>
> src/hotspot/share/code/dependencyContext.cpp line 66:
> 
>> 64: // Walk the list of dependent nmethods searching for nmethods which
>> 65: // are dependent on the changes that were passed in and mark them for
>> 66: // deoptimization.  Returns the number of nmethods found.
> 
> This part of the comment is now stale: Returns the number of nmethods found.

Fixed, thanks!

> src/hotspot/share/code/dependencyContext.cpp line 73:
> 
>> 71:     if (b->count() > 0) {
>> 72:       if (nm->is_marked_for_deoptimization()) {
>> 73:         deopt_scope->dependant(nm);
> 
> nit typo: s/dependant/dependent/

Thanks, fixed.

> src/hotspot/share/prims/methodHandles.cpp line 953:
> 
>> 951: }
>> 952: 
>> 953: void MethodHandles::flush_dependent_nmethods(DeoptimizationScope* deopt_scope, Handle call_site, Handle target) {
> 
> Is `flush_dependent_nmethods` really the right name for this method?
> Perhaps `mark_dependent_nmethods` is a better name.
> 
> Yes, I know that this name predates this PR...

Yes agreed, fixed, thanks!

> src/hotspot/share/prims/methodHandles.cpp line 1334:
> 
>> 1332:     {
>> 1333:       NoSafepointVerifier nsv;
>> 1334:       MutexLocker mu2(THREAD, CodeCache_lock, Mutex::_no_safepoint_check_flag);
> 
> Since we no longer have a `mu1`, the name shouldn't be `mu2`.
> 
> Also, the outer braces on L1330 and L1341 are not needed anymore which
> also means that the code from L1331 -> L1342 doesn't have to be indented
> the extra level.

Yes, fixed, thanks!

> src/hotspot/share/runtime/deoptimization.cpp line 131:
> 
>> 129:   Atomic::store(&cm->_deoptimization_status, status);
>> 130: 
>> 131:   // Make sure active is not committted
> 
> nit typo: s/committted/committed/

Thanks, fixed!

> src/hotspot/share/runtime/deoptimization.cpp line 150:
> 
>> 148: 
>> 149: void DeoptimizationScope::deoptimize_marked() {
>> 150:   assert(!_deopted, "Already deopt");
> 
> nit typo: s/deopt/deopted/

Fixed, thanks!

> src/hotspot/share/runtime/deoptimization.cpp line 152:
> 
>> 150:   assert(!_deopted, "Already deopt");
>> 151: 
>> 152:   // Safepoint are a special case, handled here.
> 
> nit typo: s/Safepoint/Safepoints/

Fixed, thanks!

> src/hotspot/share/runtime/deoptimization.cpp line 190:
> 
>> 188:     } else {
>> 189:       // Performs the handshake.
>> 190:       Deoptimization::deoptimize_all_marked(); // May safepoint and an additional deopt may have occured.
> 
> nit typo: s/occured/occurred/

Fixed, thanks!

> src/hotspot/share/runtime/deoptimization.cpp line 195:
> 
>> 193:         MutexLocker ml(CompiledMethod_lock->owned_by_self() ? nullptr : CompiledMethod_lock,
>> 194:                        Mutex::_no_safepoint_check_flag);
>> 195:         // Make sure that committed don't go backwards.
> 
> nit typo: s/don't/doesn't/

Fixed, thanks!

> src/hotspot/share/runtime/deoptimization.hpp line 49:
> 
>> 47:   // What gen to mark a method with, hence larger than _committed_deopt_gen.
>> 48:   static uint64_t _active_deopt_gen;
>> 49:   // Indicate a in-progress deopt handshake.
> 
> nit typo: s/a in-progress/an in-progress/

Fixed, thanks!

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

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