RFR: JDK-8171294: Slow compilation with long classpaths under JDK 9

[Jan Lahoda]

On 25.1.2017 14:42, Claes Redestad wrote:
>
>
> On 01/25/2017 02:24 PM, Jan Lahoda wrote:
>> On 25.1.2017 13:59, Claes Redestad wrote:
>>>
>>>
>>> On 01/25/2017 01:27 PM, Jan Lahoda wrote:
>>>> On 25.1.2017 12:29, Maurizio Cimadamore wrote:
>>>>>
>>>>>
>>>>> On 25/01/17 10:27, Claes Redestad wrote:
>>>>>> Hi,
>>>>>>
>>>>>> this might be a pre-existing issue, but doesn't this need to take
>>>>>> Multi-
>>>>>> Release JARs into account and walk the (most) appropriate subtree?
>>>>> My understanding (Jan corrects me if I'm wrong) is that javac
>>>>> delegates
>>>>> MR-jar questions to the underlying nio file system. So, given the
>>>>> cache
>>>>> is implemented using a nio file walker, the code should be already
>>>>> doing
>>>>> the appropriate thing?
>>>>
>>>> Yes, we defer the MR-jar handling to the JarFileSystem. I am not sure
>>>> if the handling there should be improved, but I don't think this patch
>>>> changes anything. We only keep (valid) package names, and while we
>>>> will currently only get packages in the "default" version, I believe
>>>> listing also works only for such packages currently.
>>>
>>> Right, it should work for the default case of compiling at the
>>> current JDKs
>>> runtime level, and there's code in BaseFileManager that allows setting
>>> of the
>>> "multi-release" attribute that'd instruct the JarFileSystem to do the
>>> right
>>> thing when compiling for another release...
>>>
>>> What I was wondering about is whether the env per jar file is set
>>> appropriately,
>>> i.e., does --source/--target mean we're compiling against the
>>> appropriate code,
>>> and - with this patch - do we see the appropriate set of packages in
>>> each jar?
>>
>> If the jar contains a new package specific for e.g. version 9, listing
>> of that package won't work (both before or after this patch). Not
>> completely sure if that's intended, but I think that's something that
>> should be solved in the JarFS.
>
> If packages for the current release won't show up then I'd consider that
> a bug;
> are you sure this is the case?  The alternative, that packages specific
> to 9
> would show up in the listing when compiling for --release 8 would be
> equally
> bad.
>
> JarFileSystem already has code which seems to be doing the right thing
> given
> the right environment input:
>
> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/f2325d80b37c/src/jdk.zipfs/share/classes/jdk/nio/zipfs/JarFileSystem.java#l66
>
>
> ... thus I think this is a question of whether the
> --source/--release/--target
> is (or should be) flowing into setting of the "multi-release" property.
> Obviously
> there's some value being set here, it's just not easy to see it's the
> correct one.

I've prepared a simple test here:
http://cr.openjdk.java.net/~jlahoda/MultiReleaseJar.java

Seems that javac is (both before and after this patch) behaving in line 
with what JarFileSystem is returning. AFAIK, javac is passing the 
multi-release property to JarFileSystem properly and is relying on 
JarFileSystem to handle multi-release jars. So, if a different behavior 
is desired, I think it would be good to have a bug filled against 
JarFileSystem, and have it fixed there.

Thanks,
     Jan

>
> Thanks!
>
> /Claes
>
>>
>> Jan
>>
>>>
>>> Sorry for going out on a tangent here, the patch itself looks really
>>> good to me. :-)
>>>
>>> Thanks!
>>>
>>> /Claes
>>>
>>>>
>>>> Thanks,
>>>>     Jan
>>>>
>>>>>
>>>>> Maurizio
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> /Claes
>>>>>>
>>>>>> On 2017-01-24 15:35, Jan Lahoda wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> As reported:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8171294
>>>>>>> http://mail.openjdk.java.net/pipermail/compiler-dev/2016-December/010596.html
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> javac has a problem with compiling with many jars on classpath. The
>>>>>>> problem is twofold:
>>>>>>> a) there is JavacFileManager.ArchiveContainer.pathCache, and this
>>>>>>> cache
>>>>>>> keeps entries not only for path that are in the given
>>>>>>> archive/jar, but
>>>>>>> also for those that are not in it, which may consume too much memory
>>>>>>>
>>>>>>> b) when listing a package, for each archive/jar, an equivalent of
>>>>>>> Files.exists(Path.resolve(String)) is done, which takes some
>>>>>>> time, and
>>>>>>> this is repeated a lot of times (and most of the time, the package
>>>>>>> does
>>>>>>> not exist in the given jar).
>>>>>>>
>>>>>>> The proposed patch is basically Maurizio's patch that lists
>>>>>>> packages on
>>>>>>> opening and then can quickly decide if a given archive/jar contains
>>>>>>> the
>>>>>>> given package or not. The biggest change from the original patch is
>>>>>>> that
>>>>>>> the packages are computed immediately when the ArchiveContainer is
>>>>>>> created, as opposed to compute them lazily. The reason is that the
>>>>>>> Containers are created lazily, and are used immediately after being
>>>>>>> created.
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~jlahoda/8171294/webrev.00/
>>>>>>>
>>>>>>> How does this look?
>>>>>>>
>>>>>>> Thanks,
>>>>>>>     Jan
>>>>>
>>>
>