JDK9 RFR of JDK-8029646: [pack200] should support the new zip64 format.
Kumar Srinivasan
kumar.x.srinivasan at oracle.com
Wed Jan 22 17:54:39 UTC 2014
Alex,
I am satisfied.
Thanks
Kumar
> Kumar,
>
> see my notes inline.
>
> On 1/21/14 20:11, Kumar Srinivasan wrote:
>> Alex,
>>
>> Thanks for adding the test, few comments:
>>
>> PackTestZip64.java:
>>
>> 1. compareTwoFile, I would read the entire file into
>> ByteArrayInputStream, compare
>> the total first ie. fast fail, but what you have is also ok.
> I've tried the approach of reading the entire file into memory and it
> doesn't seem to
> speed up the process so much, so i'd left it as it is.
>> 2. generateLargeJar: I would replace lines 126,127 and 128 using
>> for-each loop,
>> by using Collections.list<Enumeration> e); will make it more compact.
> Done.
>> 3. Though the test might not take too long on your system, I am
>> concerned it may
>> take too long on our Jprt system.
> I tested it on JPRT - there's no system on which test takes more than
> a minute - actually
> the longest test run time was on the solaris-sparc and it's 44
> seconds, so it seems it's Ok.
>
> The updated webrev can be found at:
> http://cr.openjdk.java.net/~kizune/8029646/webrev.03
>
> /Alex
>> Kumar
>>
>>
>> On 1/16/2014 8:21 AM, Alexander Zuev wrote:
>>> Sherman, Kumar,
>>>
>>> i have fixed the glitches you have found and changed the test so
>>> it creates a new jar
>>> based on the golden.jar content (to have a reasonable set of various
>>> data to start with),
>>> then adding to it 65536 empty files to test how we cope with such a
>>> huge jars.
>>>
>>> The testing time is less than a 30 seconds on my machine (not the
>>> top-of-the-line) so i
>>> decided not to bother with conditional testing, lets test our
>>> behavior every time in full.
>>>
>>> The updated webrev can be found at
>>> http://cr.openjdk.java.net/~kizune/8029646/webrev.02
>>>
>>> /Alex
>>>
>>> On 1/15/14 21:34, Xueming Shen wrote:
>>>> On 1/15/14 7:01 AM, Alexander Zuev wrote:
>>>>> Hello,
>>>>>
>>>>> the new webrev with all the typos and comments fixed can be
>>>>> found at http://cr.openjdk.java.net/~kizune/8029646/webrev.01/
>>>>>
>>>>> /Alex
>>>>
>>>> (1) jarmagic can be just a static constant somewhere or a stack
>>>> variable. not big deal though.
>>>> (2) the test only "tests" EOF for s. there is possibility that the
>>>> newly created has more extra
>>>> bytes at the end...though in theory this should not happen,
>>>> it might be better just add an
>>>> extra line to check the sizes of two first first?
>>>> (3) the rest of the change looks good, but I agreed with Kumar
>>>> that you may need to add a
>>>> regression test for a jar file with > 64k entries. otherwise
>>>> the code for zip64 end is not
>>>> being tested. the code looks fine, but I would trust a
>>>> regression test more than my eyes:-)
>>>>
>>>> Thanks!
>>>> -Sherman
>>>
>>
>
More information about the core-libs-dev
mailing list