[8u] RFR: Backport of JDK-8222670: pathological case of JIT recompilation and code cache bloat

Fri May 24 18:04:26 UTC 2019

On 18/05/2019 18:20, Liu, Xin wrote:
> Hi, Andrew, 
> Thanks for review it. Even though it's a backport patch, but the review workload as high as original one.  Thanks.
> My comments are inline. 
> 
> On 5/16/19, 11:47 AM, "jdk8u-dev on behalf of Andrew John Hughes" <jdk8u-dev-bounces at openjdk.java.net on behalf of gnu.andrew at redhat.com> wrote:
> 
>     On 16/05/2019 18:23, Liu, Xin wrote:
>     > Hi, Reviewers,
>     > 
>     > Could you review this backport patch?
>     > 
>     > BUG: https://bugs.openjdk.java.net/browse/JDK-8222670
>     > Webrev: https://cr.openjdk.java.net/~xliu/8222670/webrev.8u-dev/
>     > 
>     > I can’t cleanly apply the original patch to jdk8u.  I made the following changes.
>     > 
>     >   1.  Include JDK-8223537.  It solved suspend problem in blocking mode. One line change.
>     
>     I think this should be applied as a pre-requisite clean backport. Just
>     flag the bug with jdk8u-fix-request, no need for a separate review.
>     
> I added jdk8u-fix-request for JDK-8223537
> The change can trivially apply on jdk8u-dev. Here is webrev.  The order does matter,  so let's call it  3/3 patch, or last one. 
> https://cr.openjdk.java.net/~xliu/8223537/webrev.8u-dev/

Ok, this has jdk8u-fix-yes now. Not sure why the webrev still lists 8222670.

> 
>     >   2.  Shorter wait period.  Otherwise, it’s easy to timeout for the  test.  It’s part of a JDK-8066770.
>     > https://cr.openjdk.java.net/~xliu/8222670/webrev.8u-dev/test/compiler/whitebox/CompilerWhiteBoxTest.java.udiff.html
>     > 
>     
>     8066770 relies on a lot of WhiteBox API that is not present in 8u so I'm
>     ok with including just this segment.
>     
>     >   1.  +1 for Tier4MinInvocationThreshold and Tier4CompileThreshold. Otherwise, we have to backport JDK-8059604, which looks like complicated.
>     > 
>     > https://cr.openjdk.java.net/~xliu/8222670/webrev.8u-dev/src/share/vm/prims/whitebox.cpp.udiff.html
>     
>     JDK-8059604 looks backportable, but I'm ok with including this commented
>     workaround for now.
>>>
> It is backportable. Actually, Paul suggested me do it. 
> I think it's complicated because JDK-8059604 is only part of campaign(JDK-8050853). Taking 8059604 alone to jdk8u seems not be very useful. 
> Further, I can't evaluate how much impact of changing thresholds  from > to >= here.  
> https://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/ffe9c8c82350#l5.9
> 
> I'd like to +1 in whitebox.cpp because it's just a fixture of testing. It won't have runtime risk. 

Yes, I agree; do this for now and maybe look at that later.

>     
>     There seem to be two other changes being included here:
>     
>     1. compileBroker.cpp changes
>     
>     These aren't in the original JDK-8222670. In 9, the != NULL test is
>     added by "8144964: JVMCI compilations need to be disabled until the
>     module system is initialized". 8 has neither JVMCI or a module system,
>     so not sure why this test is being added here. Why is this included?
>>>    
> I do need the "task != NULL" protection here. Otherwise,  remove(NULL/*task*/); will cash JVM. 
> If duplication happens ,  this patch will ditch the task from select_task and return NULL. 
> 
> For JDK-8144964, it’s all about JVMCI. That's why I don't want to backport it.

Yes, I wasn't suggesting to do so, I just wondered why it was needed.
Thanks for clarifying.

>  
> 
>     2. skiponTieredCompilation in CompilerWhiteBoxTest.java
>     
>     This is part of "8059575: JEP-JDK-8043304: Test task: Tiered Compilation
>     level transition tests" which should also be backported as a pre-requisite.
>>>    
> I backported 8059575 to jdk8u-dev.  It's the 1/3 patch, or the first one to apply. 
> Here is webrev: https://cr.openjdk.java.net/~xliu/8059575/webrev.01/
> 
> I was wrong at JDK-8059575. It need to polish a little bit to pass all tests. 
> Here is my change: 
> 1) add compile_id in NMethod.java. 
> I got lost from here. 
> https://hg.openjdk.java.net/jdk9/jdk9/rev/c82ea5393dda

That file move obscures things and I think it silently does include the
addition of 'final'.

The additions to whitebox.cpp & NMethod.java to add compile_id come from
JDK-8054492. In both cases though, it's needed for the test, not the
actual fix. So whether you want to backport this as well is up to you.
It adds a Class.cast intrinsic.

> 2) modify @library for LevelTransitionTest.java and ConstantGettersTransitionsTest.java. 
> It's because auxiliary library locates differently. 

That's follows from not having the file movement that takes place in
8066433, 8067223 & 8067337 which look odd now they are in the single jdk
repository, but previously involved two repositories. The problems you
see with tracking the history are because the files were added again in
one changeset (to the root repository) and then the other copies removed
in another changeset.

I don't think we need to do that move as the only JDK test that
currently uses whitebox is test/jdk/java/lang/ref/CleanerTest.java and
java.lang.ref.Cleaner is part of the Java 9 API. So I'm fine with just
changing @library.

> 
> 
>     > Thanks,
>     > --lx
>     > 
>     
>     Thanks,
>     -- 
>     Andrew :)
>     
>     Senior Free Java Software Engineer
>     Red Hat, Inc. (http://www.redhat.com)
>     
>     PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
>     Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
>     https://keybase.io/gnu_andrew
>     
>     
> 

-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew