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

[Jan Lahoda]

On 25.1.2017 18:23, Jan Lahoda wrote:
> 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.

(fwiw, I can file the bug if you wish.)

Jan

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