RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Claes Redestad
Hi,

please review this change to adapt the JarFileSystem::isMultiReleaseJar
method to align with the evolved specification in JEP 238

Bug: https://bugs.openjdk.java.net/browse/JDK-8176709
Webrev: http://cr.openjdk.java.net/~redestad/8176709/jdk.01/

This unfortunately adds a qualified export from java.base to jdk.zipfs,
but since the jdk.internal.util.jar package was already exported to
three other modules I think it's a low cost option that is preferable
to other alternatives such as maintaining separate implementations.

Thanks!

/Claes
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Claes Redestad
Hi,

Alan raised some concerns offline that we should try to reduce the
number of qualified exports, not adding more, and that it might be
better to accept some code duplication here. Thus I'm proposing this as
an alternative:

http://cr.openjdk.java.net/~redestad/8176709/jdk.02/

Neither solution is exactly pretty, but this approach removes any
performance risk of jdk.01 and by at least calling out that there's
some duplication around should avoid us slipping back into a similar
situation again.

Thanks!

/Claes

On 2017-03-14 16:04, Claes Redestad wrote:

> Hi,
>
> please review this change to adapt the JarFileSystem::isMultiReleaseJar
> method to align with the evolved specification in JEP 238
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8176709
> Webrev: http://cr.openjdk.java.net/~redestad/8176709/jdk.01/
>
> This unfortunately adds a qualified export from java.base to jdk.zipfs,
> but since the jdk.internal.util.jar package was already exported to
> three other modules I think it's a low cost option that is preferable
> to other alternatives such as maintaining separate implementations.
>
> Thanks!
>
> /Claes
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Mandy Chung
I agree with the goal to reduce the number of qualified exports, which I always like to keep.

Duplicating code like this isn’t ideal although it’s straight-forward.  This is a performance optimization. One solution is to keep using the Manifest API and check the attribute value equals to `true` and separate the performance issue and explore any other solution.  Perhaps parsing of Manifest could be optimized.

Mandy


> On Mar 14, 2017, at 11:42 AM, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> Alan raised some concerns offline that we should try to reduce the
> number of qualified exports, not adding more, and that it might be
> better to accept some code duplication here. Thus I'm proposing this as
> an alternative:
>
> http://cr.openjdk.java.net/~redestad/8176709/jdk.02/
>
> Neither solution is exactly pretty, but this approach removes any
> performance risk of jdk.01 and by at least calling out that there's
> some duplication around should avoid us slipping back into a similar
> situation again.
>
> Thanks!
>
> /Claes
>
> On 2017-03-14 16:04, Claes Redestad wrote:
>> Hi,
>>
>> please review this change to adapt the JarFileSystem::isMultiReleaseJar
>> method to align with the evolved specification in JEP 238
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176709
>> Webrev: http://cr.openjdk.java.net/~redestad/8176709/jdk.01/
>>
>> This unfortunately adds a qualified export from java.base to jdk.zipfs,
>> but since the jdk.internal.util.jar package was already exported to
>> three other modules I think it's a low cost option that is preferable
>> to other alternatives such as maintaining separate implementations.
>>
>> Thanks!
>>
>> /Claes

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Claes Redestad
Hi Mandy,

On 03/15/2017 12:32 AM, Mandy Chung wrote:
> I agree with the goal to reduce the number of qualified exports, which I always like to keep.
>
> Duplicating code like this isn’t ideal although it’s straight-forward.  This is a performance optimization. One solution is to keep using the Manifest API and check the attribute value equals to `true` and separate the performance issue and explore any other solution.  Perhaps parsing of Manifest could be optimized.

yes, this enhances the performance of JarFileSystem::isMultiReleaseJar,
which isn't strictly necessary
for the bug fix at hand[1], but based on that fact that the code in
JarFile has been thoroughly tested
I figured this to be the lowest risk change rather than figuring out how
to make the current
Manifest-reading code correct.

At a glance then making the existing code correct is likely trivial and
minimal as you say, but I was
simply more comfortable copying/reusing code that I know works than
delving into the details of an
implementation with which I'm less familiar.

If you insist and can review promptly I'll go ahead and make a minimal
fix tonight, but I'd prefer to go
ahead with this well-known code before we hit RDP2 tomorrow.

Thanks!

/Claes

[1] While technically having JarFileSystem read the Manifest on creation
*is* a performance regression
since that's never done in 8, I have no proof that this is in any way a
critical regression.

>
> Mandy
>
>
>> On Mar 14, 2017, at 11:42 AM, Claes Redestad <[hidden email]> wrote:
>>
>> Hi,
>>
>> Alan raised some concerns offline that we should try to reduce the
>> number of qualified exports, not adding more, and that it might be
>> better to accept some code duplication here. Thus I'm proposing this as
>> an alternative:
>>
>> http://cr.openjdk.java.net/~redestad/8176709/jdk.02/
>>
>> Neither solution is exactly pretty, but this approach removes any
>> performance risk of jdk.01 and by at least calling out that there's
>> some duplication around should avoid us slipping back into a similar
>> situation again.
>>
>> Thanks!
>>
>> /Claes
>>
>> On 2017-03-14 16:04, Claes Redestad wrote:
>>> Hi,
>>>
>>> please review this change to adapt the JarFileSystem::isMultiReleaseJar
>>> method to align with the evolved specification in JEP 238
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8176709
>>> Webrev: http://cr.openjdk.java.net/~redestad/8176709/jdk.01/
>>>
>>> This unfortunately adds a qualified export from java.base to jdk.zipfs,
>>> but since the jdk.internal.util.jar package was already exported to
>>> three other modules I think it's a low cost option that is preferable
>>> to other alternatives such as maintaining separate implementations.
>>>
>>> Thanks!
>>>
>>> /Claes

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Mandy Chung

> On Mar 15, 2017, at 4:06 AM, Claes Redestad <[hidden email]> wrote:
>
> Hi Mandy,
>
> On 03/15/2017 12:32 AM, Mandy Chung wrote:
>> I agree with the goal to reduce the number of qualified exports, which I always like to keep.
>>
>> Duplicating code like this isn’t ideal although it’s straight-forward.  This is a performance optimization. One solution is to keep using the Manifest API and check the attribute value equals to `true` and separate the performance issue and explore any other solution.  Perhaps parsing of Manifest could be optimized.
>
> yes, this enhances the performance of JarFileSystem::isMultiReleaseJar, which isn't strictly necessary
> for the bug fix at hand[1], but based on that fact that the code in JarFile has been thoroughly tested
> I figured this to be the lowest risk change rather than figuring out how to make the current
> Manifest-reading code correct.
>
> At a glance then making the existing code correct is likely trivial and minimal as you say, but I was
> simply more comfortable copying/reusing code that I know works than delving into the details of an
> implementation with which I'm less familiar.
>
> If you insist and can review promptly I'll go ahead and make a minimal fix tonight, but I'd prefer to go
> ahead with this well-known code before we hit RDP2 tomorrow.
>
> Thanks!
>
> /Claes
>
> [1] While technically having JarFileSystem read the Manifest on creation *is* a performance regression
> since that's never done in 8, I have no proof that this is in any way a critical regression.

I would prefer to separate the performance issue from this fix and keep this fix simple.  There may be more to tease out for performance regression that I defer to Sherman to discuss with you.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Claes Redestad


On 03/15/2017 05:36 PM, Mandy Chung wrote:
> I would prefer to separate the performance issue from this fix and keep this fix simple.  There may be more to tease out for performance regression that I defer to Sherman to discuss with you.

Ok:

http://cr.openjdk.java.net/~redestad/8176709/jdk.03/

Also fixed an additional correctness issue, as we discussed offline, in
that we should only catch and swallow NoSuchFileException (when manifest
is missing), and rethrow any other IOException.

/Claes

>
> Mandy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Xueming Shen

The import j.u.Objects is no longer needed?

The rest looks fine.

-Sherman

On 3/15/17, 11:25 AM, Claes Redestad wrote:

>
>
> On 03/15/2017 05:36 PM, Mandy Chung wrote:
>> I would prefer to separate the performance issue from this fix and
>> keep this fix simple.  There may be more to tease out for performance
>> regression that I defer to Sherman to discuss with you.
>
> Ok:
>
> http://cr.openjdk.java.net/~redestad/8176709/jdk.03/
>
> Also fixed an additional correctness issue, as we discussed offline,
> in that we should only catch and swallow NoSuchFileException (when
> manifest is missing), and rethrow any other IOException.
>
> /Claes
>
>>
>> Mandy
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Mandy Chung
In reply to this post by Claes Redestad

> On Mar 15, 2017, at 11:25 AM, Claes Redestad <[hidden email]> wrote:
>
>
>
> On 03/15/2017 05:36 PM, Mandy Chung wrote:
>> I would prefer to separate the performance issue from this fix and keep this fix simple.  There may be more to tease out for performance regression that I defer to Sherman to discuss with you.
>
> Ok:
>
> http://cr.openjdk.java.net/~redestad/8176709/jdk.03/
>
> Also fixed an additional correctness issue, as we discussed offline, in that we should only catch and swallow NoSuchFileException (when manifest is missing), and rethrow any other IOException.

Looks good.  You can take out the unused import before the push.  Thanks for separating the performance fix from this patch.

Mandy
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8176709: JarFileSystem::isMultiReleaseJar is incorrect

Claes Redestad


On 03/15/2017 07:33 PM, Mandy Chung wrote:

>> On Mar 15, 2017, at 11:25 AM, Claes Redestad <[hidden email]> wrote:
>>
>>
>>
>> On 03/15/2017 05:36 PM, Mandy Chung wrote:
>>> I would prefer to separate the performance issue from this fix and keep this fix simple.  There may be more to tease out for performance regression that I defer to Sherman to discuss with you.
>> Ok:
>>
>> http://cr.openjdk.java.net/~redestad/8176709/jdk.03/
>>
>> Also fixed an additional correctness issue, as we discussed offline, in that we should only catch and swallow NoSuchFileException (when manifest is missing), and rethrow any other IOException.
> Looks good.

Thanks!

>   You can take out the unused import before the push.

Also updated copyrights. Pushed.

/Claes

>    Thanks for separating the performance fix from this patch.
>
> Mandy

Loading...