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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Jan Lahoda
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
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad
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?

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
Reply | Threaded
Open this post in threaded view
|

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

Maurizio Cimadamore


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?

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

Reply | Threaded
Open this post in threaded view
|

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

Jan Lahoda
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.

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
>
Reply | Threaded
Open this post in threaded view
|

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

Claes Redestad


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?

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

Reply | Threaded
Open this post in threaded view
|

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

Jan Lahoda
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. (Adding Steve on CC.)

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
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

Re: 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
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: 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
>>>>>>
>>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Jonathan Gibbons
In reply to this post by Claes Redestad


On 01/25/2017 05:42 AM, 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.
>
> Thanks!
>
> /Claes


Generally the spec for javac with regard to multi-release jars is that
javac will pass down the
effective target level, and otherwise totally delegate to the
JarFileSystem as to what packages
should be listed.

I do not think javac (or any other tools, except maybe the jar tool)
should be in the business of
understanding the internal structure and constraints of multi-release jars.

-- Jon

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

Reply | Threaded
Open this post in threaded view
|

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

Jonathan Gibbons
In reply to this post by Jan Lahoda


On 01/24/2017 06:35 AM, 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

The basic patch looks look.

The extended discussion with respect to multi-release jars, and whether
the jar file system is
reporting the correct packages to javac is a separate discussion.

-- Jon