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

[Daniel D. Daugherty]

On Thu, 16 Feb 2023 08:38:42 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

I have a bunch of editorial nits from my first read thru. I'm going to post
those first and then do another read thru to try and make sense of the
logic changes.

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.

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/

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

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.

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

> 121:   // If it's already marked but we still need it to be deopted.
> 122:   if (cm->is_marked_for_deoptimization()) {
> 123:     dependant(cm);

nit typo: s/dependant/dependent/

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

> 137: }
> 138: 
> 139: void DeoptimizationScope::dependant(CompiledMethod* cm) {

nit typo: s/dependant/dependent/

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

> 148: 
> 149: void DeoptimizationScope::deoptimize_marked() {
> 150:   assert(!_deopted, "Already deopt");

nit typo: s/deopt/deopted/

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/

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/

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/

src/hotspot/share/runtime/deoptimization.hpp line 59:

> 57:   DeoptimizationScope();
> 58:   ~DeoptimizationScope();
> 59:   // Mark a method, if already marked as dependant.

nit typo: s/dependant/dependent/

src/hotspot/share/runtime/deoptimization.hpp line 61:

> 59:   // Mark a method, if already marked as dependant.
> 60:   void mark(CompiledMethod* cm, bool inc_recompile_counts = true);
> 61:   // Record this as a dependant method.

nit typo: s/dependant/dependent/

src/hotspot/share/runtime/deoptimization.hpp line 65:

> 63: 
> 64:   // Execute the deoptimization.
> 65:   // Mmake the nmethods not entrant, stackwalks and patch return pcs and sets post call nops.

nit typo: s/Mmake/Make/

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

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