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 <claes.redes...@oracle.com> 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
