> On Mar 15, 2017, at 4:06 AM, Claes Redestad <claes.redes...@oracle.com> 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
