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

[Claes Redestad]

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.

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