Odd interaction between ArrayList$Itr and Escape Analysis
Krystal Mok
rednaxelafx at gmail.com
Wed Sep 28 16:42:34 UTC 2016
Hi Vladimir,
Yes, the patch I posted was the short-term one that I used for getting rid
of this particular kind of problem before a release, and it's already in
production for us. So it was deliberately focused on a very narrow scenario
so that I don't have to worry about testing too much.
I do also have another patch for the general case for "unused unloaded
arguments". I haven't gotten around to polish and test that patch yet, but
since we're seeing a good motivation on the OpenJDK side as well, I may as
well go back and get that patch ready soon.
A null guard is a good way to go. It's basically the same kind of logic
that C2 OSR entry already uses. In this case, at a call site, a null guard
on the caller-side against an argument whose type is unloaded is one way to
do it.
(There are of course other alternatives. e.g. If we focus on the
callee-side, in a compiler with a mixed top-down / bottom-up inlining
heuristics system, the (devirtualized if needed) callee can be inspected
first to see if an argument of unloaded type is never used or not. If it is
never used, don't even bother inserting the null guard on the caller-side,
and just go ahead and inline would be good and safe. C2 doesn't have this
luxury yet so tackling the problem with a caller-side solution is easier to
do.)
IMO a nmethod dependency on an "unloaded class" isn't that feasible, since
you might not even have a concrete entity to "depend on", and registering
symbolic dependencies for "unloaded classes" in general, even though I
believe is doable, might be rather tedious.
Thanks,
Kris
On Wed, Sep 28, 2016 at 9:17 AM, Vladimir Ivanov <
vladimir.x.ivanov at oracle.com> wrote:
> Kris, thanks for sharing the patch!
>
> IMO the problem we observe is not specific to bridge methods.
>
> It demonstrates a generic short-coming in C2 inlining heuristic: even
> though the argument is never used (otherwise, the class would have been
> already loaded, right?), we don't inline the whole method.
>
> So, I'd prefer to see a solution which covers the general case.
>
> Can we do that? It seems so: it could be achieved by a null guard on the
> argument or a nmethod dependency on the unloaded class.
>
> Best regards,
> Vladimir Ivanov
>
> On 9/28/16 6:37 PM, Krystal Mok wrote:
>
>> Hi guys,
>>
>> Here's the HotSpot-side patch, based on OpenJDK9 HotSpot:
>>
>> Webrev: http://cr.openjdk.java.net/~kmo/8166840/webrev.00/
>>
>> Please give me a preliminary idea of how you guys feel about the patch,
>> and then I'll start an actual review thread if people agree on the
>> direction of this patch.
>>
>> Note: This is the way javac constructs that "XXX$1" name for the
>> accessConstructorTag:
>> JDK7u: http://hg.openjdk.java.net/jdk7u/jdk7u/langtools/file/93a278
>> 8178e6/src/share/classes/com/sun/tools/javac/comp/Lower.java#l1154
>> JDK9:
>> http://hg.openjdk.java.net/jdk9/jdk9/langtools/file/9f610042
>> 70d8/src/jdk.compiler/share/classes/com/sun/tools/javac/
>> comp/Lower.java#l1241
>>
>> So name matching on "$1" suffix is sufficient here to workaround this
>> particular pattern from javac.
>>
>> P.S. I haven't built OpenJDK9 in quite a while now, and apparently the
>> makefiles have changed and the scripts that I used to build JDK7u /
>> JDK8u doesn't work on JDK9. What's the current recommended way to build
>> just HotSpot with fastdebug / product levels?
>>
>> Thanks,
>> Kris (OpenJDK username: kmo)
>>
>> On Wed, Sep 14, 2016 at 3:12 AM, Vladimir Ivanov
>> <vladimir.x.ivanov at oracle.com <mailto:vladimir.x.ivanov at oracle.com>>
>> wrote:
>>
>> Kris,
>>
>> And I'm happy to upstream that patch, if the team is interested.
>>
>>
>> Sure, we are definitely interested in fixing that. Feel free to file
>> a bug and send the fix out for review.
>>
>> Now, when I first discovered the problem, my first intuition was
>> that
>> it's better to "fix" it in javac. But before nest mates in the
>> Class
>> file, there isn't much that javac could do. Changing the Java
>> libraries
>> to not use private constructors in inner classes is also doable,
>> but
>> needs changing a lot of files.
>>
>>
>> I agree that javac is not the best place to fix the immediate
>> problem: it requires recompilation and there are already lots of
>> problematic bytecode shapes out in the wild. The JVM should optimize
>> for that case instead.
>>
>> So I ended up fixing it in the VM, even though I agree fully
>> with what
>> Rémi brought up.
>>
>>
>> I'm curious how did you fix it. I haven't found a description in the
>> thread.
>>
>> It's possible to force class loading, but I'm worried about
>> undesirable effects of class initialization. Is it enough for C2 to
>> have the class loaded but not initialized to make it work?
>>
>> Another approach would be to issue a null check and deoptimize (for
>> bridge methods, the check collapses after inlining since the
>> argument is always null) or add a nmethod dependency and throw away
>> the code when the parameter class is loaded.
>>
>> Best regards,
>> Vladimir Ivanov
>>
>> The access constructor tag thingy in javac is really a weird
>> hack. If
>> you guys ever look at the contents of ArrayList$1, it's really
>> empty
>> -- the class doesn't even declare some of the usual structures in
>> a
>> normal Class file... Hopefully we can get rid of it in javac soon.
>>
>>
>> On Tuesday, September 13, 2016, Vitaly Davidovich
>> <vitalyd at gmail.com <mailto:vitalyd at gmail.com>
>> <mailto:vitalyd at gmail.com <mailto:vitalyd at gmail.com>>> wrote:
>>
>>
>>
>> On Tuesday, September 13, 2016, Remi Forax
>> <forax at univ-mlv.fr <mailto:forax at univ-mlv.fr>
>> <javascript:_e(%7B%7D,'cvml','forax at univ-mlv.fr
>> <mailto:forax at univ-mlv.fr>');>> wrote:
>>
>> I've always found that the empty inner classes generated
>> by
>> javac as a kind of hack.
>>
>> These classes should be removed in Java 10, thanks to the
>> nestmate attributes.
>>
>>
>> http://mail.openjdk.java.net/pipermail/valhalla-spec-experts
>> /2016-January/000060.html
>> <http://mail.openjdk.java.net/pipermail/valhalla-spec-expert
>> s/2016-January/000060.html>
>>
>> <http://mail.openjdk.java.net/pipermail/valhalla-spec-expert
>> s/2016-January/000060.html
>> <http://mail.openjdk.java.net/pipermail/valhalla-spec-expert
>> s/2016-January/000060.html>>
>>
>> The other solution, is to have an empty class in the jdk
>> which
>> is not visible from javac (the class itself can be marked
>> as
>> synthetic),
>> so javac can use it without creating method clash.
>>
>> and to solve the problem now, the easy solution is to add
>> a
>> package private constructor in ArrayList.Itr,
>>
>> I'm hoping Oracle can take Kris' (Azul) patch (or do something
>> similar). It might catch more cases than just modifying Itr.
>>
>>
>> private class Itr implements Iterator<E> {
>> int cursor; // index of next element to return
>> int lastRet = -1; // index of last element returned; -1
>> if no such
>> int expectedModCount = modCount;
>>
>> Itr() {
>> // avoid to generate a synthetic accessor constructor
>> }
>> }
>>
>>
>> regards,
>> Rémi
>>
>>
>> ------------------------------------------------------------
>> ------------
>>
>> *De: *"Vitaly Davidovich" <vitalyd at gmail.com
>> <mailto:vitalyd at gmail.com>>
>> *À: *"Krystal Mok" <rednaxelafx at gmail.com
>> <mailto:rednaxelafx at gmail.com>>
>> *Cc: *"hotspot compiler"
>> <hotspot-compiler-dev at openjdk.java.net
>> <mailto:hotspot-compiler-dev at openjdk.java.net>>
>> *Envoyé: *Lundi 12 Septembre 2016 22:15:41
>> *Objet: *Re: Odd interaction between ArrayList$Itr and
>>
>> Escape Analysis
>>
>>
>>
>> On Mon, Sep 12, 2016 at 3:56 PM, Krystal Mok
>> <rednaxelafx at gmail.com
>> <mailto:rednaxelafx at gmail.com>> wrote:
>>
>> On Mon, Sep 12, 2016 at 12:38 PM, Vitaly
>> Davidovich
>> <vitalyd at gmail.com <mailto:vitalyd at gmail.com>>
>>
>> wrote:
>>
>> It seems odd to me as well why inlining
>> won't force
>> load the missing class(es). If we're
>> inlining, it
>> means the method itself or the call chain
>> it's part
>> of is hot - failing to inline can have
>> negative
>> side-effects, like this example. I suppose
>> there
>> must be a good reason why it doesn't do this
>> though?
>>
>>
>> That's because we can't. The JIT compilers are
>> running
>> on their own threads, and they're not real "Java
>> threads". So they are not allowed to run
>> arbitrary Java
>> code. But Java class loading may involve running
>> arbitrary Java code, e.g. the
>> ClassLoader.loadClass()
>> upcall.
>> Force class loading can be done on the
>> triggering side
>> (for the top-level method), because compilation
>> tasks
>> are triggered from real Java threads, and they're
>> allowed to run arbitrary Java code.
>>
>> I see, makes sense. Perhaps there can be an option
>> to turn
>> on loading of required types in the entire
>> compilation unit,
>> after all inlining is done (and therefore make the
>> unloaded
>> types not be barriers for inlining). I'd personally
>> prefer
>> that over having odd performance differences.
>>
>>
>> - Kris
>>
>> Thanks Kris.
>>
>>
>>
>> --
>> Sent from my phone
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160928/87f766d5/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list