On Fri, 17 Feb 2023 14:58:06 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comment - move the BufferedOutputStream creation to Jarsigner class
>
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line
> 1970:
>
>> 1968: OutputStream os = null;
>> 1969: try {
>> 1970: os = new BufferedOutputStream(new
>> FileOutputStream(signedJarFile));
>
> Can we make this change right at
> https://github.com/openjdk/jdk/blob/5dfc4ec7d94af9fe39fdee9d83b06101b827a3c6/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java#L691
> or at least inside that class?
Hello Max, what you suggest sounds fine to me. I've updated the PR to follow
this suggestion. The only difference now would be that in the previous proposed
patch if any exception happened during the jarsiging the `BufferedOutputStream`
would be (flushed and) closed from the finally block of the main method.
However, in the newer version, there can be a case that if an exception occurs
then the underlying BufferedOutputStream, constructed in the `JarSigner` class
may not be flushed/closed, but that I think should be fine, because we anyway
delete the temporary signed jar file, in the main method, if any exception
occurs.
With this new change, I again ran the test against a 3GB and 6GB file and the
numbers continue to show the improvement. I'll trigger the tier testing too.
-------------
PR: https://git.openjdk.org/jdk/pull/12588
