Re: RFR: JDK-8152622: tools/pack200/Pack200Props.java timed out
On 06/04/2016 15:20, Kumar Srinivasan wrote: Alan, http://cr.openjdk.java.net/~ksrini/8152622/webrev.02/ I made the changes as you suggested below, I have retained Files.createDirectories, here is why, changing it to createDirectory will throw FileAlreadyExistsException, which means, that if the output zip/jar file exists then an exception handler is required to ignore it. I don't think its worth the trouble. Okay, although each directory is only visited once so I don't immediately see how the FileAlreadyExistsException arises, unless this related to sym links in the jrt file system. In any case, the updated changes looking fine. I think I would still reduce some of the really long lines to make it easier for reviewers in the future. -Alan
Re: RFR: JDK-8152622: tools/pack200/Pack200Props.java timed out
Alan, http://cr.openjdk.java.net/~ksrini/8152622/webrev.02/ I made the changes as you suggested below, I have retained Files.createDirectories, here is why, changing it to createDirectory will throw FileAlreadyExistsException, which means, that if the output zip/jar file exists then an exception handler is required to ignore it. I don't think its worth the trouble. Thanks Kumar On 06/04/2016 14:09, Kumar Srinivasan wrote: - Is Files.createDirectories needed? The walk is specified to be depth first so you'll also visit parent directories first. Yes, zipfs does not allow me to create the file without creating the enclosing directory first, so if I were to do this in visitFile, then presumably I would have to add a check, to prevent duplicate creation going into the provider, only to find a directory already exists and return, not sure if this is what you want, because preVisitDirectory does this conveniently. I should have been clearer, I was trying to say that createDirectory should be sufficient here because the walk is specified to be depth first. - I'm also curious about the REPLACE_EXISTING as I assume that isn't needed. Oh! while I was developing in NB as a discrete/stand alone project, I was reusing the same zip/jar output file, will leave it as-is, no harm, right ?. No harm, it just caught my eye as not needed. -Alan
Re: RFR: JDK-8152622: tools/pack200/Pack200Props.java timed out
On 06/04/2016 14:09, Kumar Srinivasan wrote: - Is Files.createDirectories needed? The walk is specified to be depth first so you'll also visit parent directories first. Yes, zipfs does not allow me to create the file without creating the enclosing directory first, so if I were to do this in visitFile, then presumably I would have to add a check, to prevent duplicate creation going into the provider, only to find a directory already exists and return, not sure if this is what you want, because preVisitDirectory does this conveniently. I should have been clearer, I was trying to say that createDirectory should be sufficient here because the walk is specified to be depth first. - I'm also curious about the REPLACE_EXISTING as I assume that isn't needed. Oh! while I was developing in NB as a discrete/stand alone project, I was reusing the same zip/jar output file, will leave it as-is, no harm, right ?. No harm, it just caught my eye as not needed. -Alan
Re: RFR: JDK-8152622: tools/pack200/Pack200Props.java timed out
Alan,
Thanks for the review. Please see inline comments.
Creating a zip file from the classes in the jrt file system make
sense, and should be fast.
A few comments on Utils.JrtToZip:
- the URI creation in the run() method isn't reliable, could you use
this instead:
URI uri = URI.create("jar:" + outFile.toURI());
Ok
- In the toZipFs method then I assume you should use
URI.create("jrt:/"), best to stay away from URL.
Ok
- Is Files.createDirectories needed? The walk is specified to be depth
first so you'll also visit parent directories first.
Yes, zipfs does not allow me to create the file without creating the
enclosing directory
first, so if I were to do this in visitFile, then presumably I would
have to add a
check, to prevent duplicate creation going into the provider, only to find a
directory already exists and return, not sure if this is what you want,
because
preVisitDirectory does this conveniently.
- I'm also curious about the REPLACE_EXISTING as I assume that isn't
needed.
Oh! while I was developing in NB as a discrete/stand alone project, I
was reusing the
same zip/jar output file, will leave it as-is, no harm, right ?.
A minor comment is that "root" rather than "p" would be nicer for the
root directory. Personally I would avoid the really long lines as it
makes future side-by-side reviews harder. Also throws Exception seems
too broad but it's test code so not a big deal.
Ok
Thanks
Kumar
-Alan.
Re: RFR: JDK-8152622: tools/pack200/Pack200Props.java timed out
On 06/04/2016 04:47, Kumar Srinivasan wrote:
Hi,
I made some adjustments, based on private feedback.
New Webrev:
http://cr.openjdk.java.net/~ksrini/8152622/webrev.01/
Changes
* Modified ModuleAttributes test to use the new utility method,
this also requires a way to filter/select filename patterns.
* Use preVisitDirectory to create directories once, and ignore
/packages directories.
Creating a zip file from the classes in the jrt file system make sense,
and should be fast.
A few comments on Utils.JrtToZip:
- the URI creation in the run() method isn't reliable, could you use
this instead:
URI uri = URI.create("jar:" + outFile.toURI());
- In the toZipFs method then I assume you should use
URI.create("jrt:/"), best to stay away from URL.
- Is Files.createDirectories needed? The walk is specified to be depth
first so you'll also visit parent directories first.
- I'm also curious about the REPLACE_EXISTING as I assume that isn't needed.
A minor comment is that "root" rather than "p" would be nicer for the
root directory. Personally I would avoid the really long lines as it
makes future side-by-side reviews harder. Also throws Exception seems
too broad but it's test code so not a big deal.
-Alan.
Re: RFR: JDK-8152622: tools/pack200/Pack200Props.java timed out
Hi, I made some adjustments, based on private feedback. New Webrev: http://cr.openjdk.java.net/~ksrini/8152622/webrev.01/ Changes * Modified ModuleAttributes test to use the new utility method, this also requires a way to filter/select filename patterns. * Use preVisitDirectory to create directories once, and ignore /packages directories. Thanks Kumar sorry missed the subject. Kumar Hi, The issue here was jimage extract was taking too long, exploding the modules onto certain slow filesystems, thus causing test timeouts. Now using jrtfs to zipfs, it takes a small fraction of that time. Please review: http://cr.openjdk.java.net/~ksrini/8152622/webrev.00/ Fix for: https://bugs.openjdk.java.net/browse/JDK-8152622 Thanks Kumar
Re: RFR: JDK-8152622: tools/pack200/Pack200Props.java timed out
sorry missed the subject. Kumar Hi, The issue here was jimage extract was taking too long, exploding the modules onto certain slow filesystems, thus causing test timeouts. Now using jrtfs to zipfs, it takes a small fraction of that time. Please review: http://cr.openjdk.java.net/~ksrini/8152622/webrev.00/ Fix for: https://bugs.openjdk.java.net/browse/JDK-8152622 Thanks Kumar
