[8u] RFR: Backport of JDK-8222670: pathological case of JIT recompilation and code cache bloat
Liu, Xin
xxinliu at amazon.com
Sat May 18 17:20:37 UTC 2019
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/
> 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.
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.
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
2) modify @library for LevelTransitionTest.java and ConstantGettersTransitionsTest.java.
It's because auxiliary library locates differently.
> 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
More information about the jdk8u-dev
mailing list