Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On 19/04/2018 13:16, David Lloyd wrote: OK thanks for the update. It seems odd to me - C99 has been around for a pretty long time - but, as long as the change goes in, I'm happy. Thanks! I think this is about the compiler options that are used by the build, specifically when compiling JNI code. It came up recently on build-dev too and I thought there was a JIRA issue created to re-examine them. -Alan
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
OK thanks for the update. It seems odd to me - C99 has been around for a pretty long time - but, as long as the change goes in, I'm happy. Thanks! On Thu, Apr 19, 2018 at 1:13 AM, Xueming Shen wrote: > David, > > webrev has been updated to address the compiler error on solaris-sparcv9. > The C comipler on > solaris-sparcv9 platform does not like those "declaration follow a > statement" new code in > Deflater.c and Inflater.c > > http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/ > > Thanks, > Sherman > > > On 4/18/18, 1:16 PM, David Lloyd wrote: > > +1 from me, thanks! > > On Wed, Apr 18, 2018 at 3:09 PM, Xueming Shen > wrote: > > Alan, David, > > Any more update/comment/suggestion on this one? I have updated > DeInflate.java with > some new test cases to cover the newly added methods. Good to go? > > -Sherman > > On 04/10/2018 08:49 PM, Xueming Shen wrote: > > > http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/ > > Thanks, > Sherman > > > > -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
David, webrev has been updated to address the compiler error on solaris-sparcv9. The C comipler on solaris-sparcv9 platform does not like those "declaration follow a statement" new code in Deflater.c and Inflater.c http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/ Thanks, Sherman On 4/18/18, 1:16 PM, David Lloyd wrote: +1 from me, thanks! On Wed, Apr 18, 2018 at 3:09 PM, Xueming Shen wrote: Alan, David, Any more update/comment/suggestion on this one? I have updated DeInflate.java with some new test cases to cover the newly added methods. Good to go? -Sherman On 04/10/2018 08:49 PM, Xueming Shen wrote: http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/ Thanks, Sherman
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
+1 from me, thanks! On Wed, Apr 18, 2018 at 3:09 PM, Xueming Shen wrote: > Alan, David, > > Any more update/comment/suggestion on this one? I have updated > DeInflate.java with > some new test cases to cover the newly added methods. Good to go? > > -Sherman > > On 04/10/2018 08:49 PM, Xueming Shen wrote: >> >> >> >> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/ >> >> Thanks, >> Sherman >> > -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Alan, David, Any more update/comment/suggestion on this one? I have updated DeInflate.java with some new test cases to cover the newly added methods. Good to go? -Sherman On 04/10/2018 08:49 PM, Xueming Shen wrote: http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/ Thanks, Sherman
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
That's great news, thanks! I'll pull down your updates just so my
local copy does not get out of date in the meantime.
On Tue, Apr 10, 2018 at 10:49 PM, Xueming Shen wrote:
> Hi David,
>
> The CSR has been approved
> https://bugs.openjdk.java.net/browse/JDK-8200527
>
> API docs have been updated slightly based on the review suggestion.
>
> (1) added some words in the class spec for both Inflater and Deflater.
>
> *
> * This class deflates sequences of bytes into ZLIB compressed data format.
> * The input byte sequence is provided in either byte array or byte buffer,
> * via one of the {@code setInput()} methods. The output byte sequence is
> * written to the output byte array or byte buffer passed to the
> * {@code deflate()} methods.
> *
>
> *
> * This class inflates sequences of ZLIB compressed bytes. The input byte
> * sequence is provided in either byte array or byte buffer, via one of the
> * {@code setInput()} methods. The output byte sequence is written to the
> * output byte array or byte buffer passed to the {@code deflate()} methods.
> *
>
>
> (2) adjusted the workding a little for those setInput() methods
>
> *
> * One of the {@code setInput()} methods should be called whenever
> * {@code needsInput()} returns true indicating that more input data
> * is required.
> *
>
>
> Two issues have been noticed when running tier1/2/3 tests
>
> (1) there is a error at ln#Inflater.c#243, the "input" is being released
> instead of "output"
>
> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev.00/src/java.base/share/native/libzip/Inflater.c.sdiff.html
>
> which triggered crash for some tests. fixed.
>
> (2) sun/nio/ch/TestMaxCachedBufferSize.java failed "because of" the
> "defaultBuf"
> uses direct ByteBuffer. This is probably the issue of the test but I
> simply update
> the "defaultBuf" to be the heap buffer/0, instead of touch the failed
> test case.
> I don't have problem if you prefer to "fix" the test and keep the
> "defaultBuf" as
> direct buffer instead.
>
>// static final ByteBuffer defaultBuf = ByteBuffer.allocateDirect(0);
> static final ByteBuffer defaultBuf = ByteBuffer.allocate(0);
>
> (3) I also updated test/jdk/java/util/zip/DeInflate.java with more tests for
> the new APIs.
> More tests might be desired though.
>
> The latest webrev is at
>
> http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/
>
> Thanks,
> Sherman
>
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Hi David,
The CSR has been approved
https://bugs.openjdk.java.net/browse/JDK-8200527
API docs have been updated slightly based on the review suggestion.
(1) added some words in the class spec for both Inflater and Deflater.
|*
* This class deflates sequences of bytes into ZLIB compressed data format.
* The input byte sequence is provided in either byte array or byte buffer,
* via one of the {@code setInput()} methods. The output byte sequence is
* written to the output byte array or byte buffer passed to the
* {@code deflate()} methods.
*|
|*
* This class inflates sequences of ZLIB compressed bytes. The input byte
* sequence is provided in either byte array or byte buffer, via one of the
* {@code setInput()} methods. The output byte sequence is written to the
* output byte array or byte buffer passed to the {@code deflate()} methods.
*|
(2) adjusted the workding a little for those setInput() methods
| *
* One of the {@code setInput()} methods should be called whenever
* {@code needsInput()} returns true indicating that more input data
* is required.
*|
Two issues have been noticed when running tier1/2/3 tests
(1) there is a error at ln#Inflater.c#243, the "input" is being released
instead of "output"
http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev.00/src/java.base/share/native/libzip/Inflater.c.sdiff.html
which triggered crash for some tests. fixed.
(2) sun/nio/ch/TestMaxCachedBufferSize.java failed "because of" the
"defaultBuf"
uses direct ByteBuffer. This is probably the issue of the test but
I simply update
the "defaultBuf" to be the heap buffer/0, instead of touch the
failed test case.
I don't have problem if you prefer to "fix" the test and keep the
"defaultBuf" as
direct buffer instead.
// static final ByteBuffer defaultBuf = ByteBuffer.allocateDirect(0);
static final ByteBuffer defaultBuf = ByteBuffer.allocate(0);
(3) I also updated test/jdk/java/util/zip/DeInflate.java with more tests
for the new APIs.
More tests might be desired though.
The latest webrev is at
http://cr.openjdk.java.net/~sherman/6341887.David.Lloyd/webrev/
Thanks,
Sherman
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Fri, Mar 30, 2018 at 12:05 PM, Xueming Shen wrote: > > Hi David, > > (1) Deflater.setDictionary(ByteBuffer) missed a "@since 11" Oops, easy fix. > (2) infalte(...) may not need to catch DFE twice, better (?) to just update > the inputPos/input > at outsider catch as > > if (input == null) > this.inputPos = inputPos + inputConsumed; > else > input.position(inputPos + inputConsumed; > > the if/else might be repetitive, but better than two layers of same DFE > catch? I'm okay with anything at this point, but this does introduce a "inputPos possibly not initialized" problem which doesn't seem to have a clean solution. WDYT? -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Hi David, (1) Deflater.setDictionary(ByteBuffer) missed a "@since 11" (2) infalte(...) may not need to catch DFE twice, better (?) to just update the inputPos/input at outsider catch as if (input == null) this.inputPos = inputPos + inputConsumed; else input.position(inputPos + inputConsumed; the if/else might be repetitive, but better than two layers of same DFE catch? -sherman On 3/30/18, 9:54 AM, David Lloyd wrote: Thanks! On Fri, Mar 30, 2018 at 11:52 AM, Xueming Shen wrote: On 3/30/18, 7:07 AM, Alan Bateman wrote: On 29/03/2018 13:18, David Lloyd wrote: : OK great. In that case, I think all feedback has been accounted for, and this should be ready to go AFAIK. I skimmed through the patch attached to your last mail. I also saw Sherman's mail offering to look at the existing wording about the flush marker. So I think this the API is good and we should get the CSR submitted. I'm less sure about the tests. The patch modifies FlaterTest but it's not clear that is covers all the scenarios, ReadOnlyBufferException just one that comes to mind. Maybe we could identify additional tests while the CSR is in progress. -Alan CSR: https://bugs.openjdk.java.net/browse/JDK-8200527
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Thanks! On Fri, Mar 30, 2018 at 11:52 AM, Xueming Shen wrote: > On 3/30/18, 7:07 AM, Alan Bateman wrote: >> >> On 29/03/2018 13:18, David Lloyd wrote: >>> >>> : >>> OK great. In that case, I think all feedback has been accounted for, >>> and this should be ready to go AFAIK. >>> >> I skimmed through the patch attached to your last mail. I also saw >> Sherman's mail offering to look at the existing wording about the flush >> marker. So I think this the API is good and we should get the CSR submitted. >> >> I'm less sure about the tests. The patch modifies FlaterTest but it's not >> clear that is covers all the scenarios, ReadOnlyBufferException just one >> that comes to mind. Maybe we could identify additional tests while the CSR >> is in progress. >> >> -Alan > > CSR: https://bugs.openjdk.java.net/browse/JDK-8200527 -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On 3/30/18, 7:07 AM, Alan Bateman wrote: On 29/03/2018 13:18, David Lloyd wrote: : OK great. In that case, I think all feedback has been accounted for, and this should be ready to go AFAIK. I skimmed through the patch attached to your last mail. I also saw Sherman's mail offering to look at the existing wording about the flush marker. So I think this the API is good and we should get the CSR submitted. I'm less sure about the tests. The patch modifies FlaterTest but it's not clear that is covers all the scenarios, ReadOnlyBufferException just one that comes to mind. Maybe we could identify additional tests while the CSR is in progress. -Alan CSR: https://bugs.openjdk.java.net/browse/JDK-8200527
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On 29/03/2018 13:18, David Lloyd wrote: : OK great. In that case, I think all feedback has been accounted for, and this should be ready to go AFAIK. I skimmed through the patch attached to your last mail. I also saw Sherman's mail offering to look at the existing wording about the flush marker. So I think this the API is good and we should get the CSR submitted. I'm less sure about the tests. The patch modifies FlaterTest but it's not clear that is covers all the scenarios, ReadOnlyBufferException just one that comes to mind. Maybe we could identify additional tests while the CSR is in progress. -Alan
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Wed, Mar 28, 2018 at 11:38 PM, Xueming Shen wrote: > On 3/28/18, 6:14 AM, David Lloyd wrote: >> I've implemented all the other changes except for this one. Latest >> version is attached, online version is here [1]. > > "flush marker" was copy/pasted from the original zlib.h api doc when I added > the > support for different flush options. It might be better to put it is a > apiNote. A > "flush marker" in this case is a 5-byte empty block, which will be output if > the > deflater does not have enough space to output the bytes and it is in > full_flush or > sync_flush mode. I think you can leave it as is for now and I will try to see > if I > can move it around or add some understandable wording, if I can. OK great. In that case, I think all feedback has been accounted for, and this should be ready to go AFAIK. Thanks! -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On 3/28/18, 6:14 AM, David Lloyd wrote: The current wording (which pre-dates your changes of course) reads more like API advice. I think it's a bit confusing too as it doesn't define what a "flush marker" is. I think we will need to re-word that sentence to make it clearer. OK. I am at a loss for a better way to explain it though; any suggestions? I've implemented all the other changes except for this one. Latest version is attached, online version is here [1]. "flush marker" was copy/pasted from the original zlib.h api doc when I added the support for different flush options. It might be better to put it is a apiNote. A "flush marker" in this case is a 5-byte empty block, which will be output if the deflater does not have enough space to output the bytes and it is in full_flush or sync_flush mode. I think you can leave it as is for now and I will try to see if I can move it around or add some understandable wording, if I can. -Sherman
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Tue, Mar 27, 2018 at 11:59 AM, David Lloyd wrote:
> On Tue, Mar 27, 2018 at 10:10 AM, Alan Bateman
> wrote:
>> On 27/03/2018 00:09, David Lloyd wrote:
>>> I was going for some kind of consistency with the array variants (that
>>> is, name the parameter what it is), though they simply use "b" for
>>> their parameter name so that interpretation might be a stretch.
>>> Should I update them all?
>>
>> I think this would be good, if you don't mind doing it.
>
> OK.
>
>>> I'm not 100% familiar with the new JavaDoc categories (ok I'm 0%
>>> familiar), but the JEP for these says "This category consists of
>>> commentary, rationale, or examples pertaining to the API.". But this
>>> feels more like specification to me since it is "specifications that
>>> apply to all valid implementations, including preconditions,
>>> postconditions, etc.". Which is to say, if you don't provide enough
>>> remaining space in the output buffer, you will have incorrect
>>> operation as a result. WDYT?
>>
>> The current wording (which pre-dates your changes of course) reads more like
>> API advice. I think it's a bit confusing too as it doesn't define what a
>> "flush marker" is. I think we will need to re-word that sentence to make it
>> clearer.
>
> OK. I am at a loss for a better way to explain it though; any suggestions?
I've implemented all the other changes except for this one. Latest
version is attached, online version is here [1].
[1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v18
--
- DML
commit 55e4df2ab3c7ccb68426bc90f3cc5d2d1a3de96e
Author: David M. Lloyd
Date: Fri Feb 16 11:00:10 2018 -0600
[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer
diff --git a/make/mapfiles/libzip/mapfile-vers
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
- Java_java_util_zip_Deflater_deflateBytes;
+ Java_java_util_zip_Deflater_deflateBytesBytes;
+ Java_java_util_zip_Deflater_deflateBytesBuffer;
+ Java_java_util_zip_Deflater_deflateBufferBytes;
+ Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+ Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
- Java_java_util_zip_Inflater_inflateBytes;
+ Java_java_util_zip_Inflater_inflateBytesBytes;
+ Java_java_util_zip_Inflater_inflateBytesBuffer;
+ Java_java_util_zip_Inflater_inflateBufferBytes;
+ Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+ Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..85e3debb393 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,7 +26,13 @@
package java.util.zip;
import java.lang.ref.Cleaner.Cleanable;
+import java.lang.ref.Reference;
+import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
+import java.util.Objects;
+
import jdk.internal.ref.CleanerFactory;
+import sun.nio.ch.DirectBuffer;
/**
* This class provides support for general purpose compression using the
@@ -92,8 +98,9 @@ import jdk.internal.ref.CleanerFactory;
public class Deflater {
private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input = ZipUtils.defaultBuf;
+private byte[] inputArray;
+private int inputPos, inputLim;
private int level, strategy;
private boolean setParams;
private boolean finish, finished;
@@ -170,9 +177,14 @@ public class Deflater {
*/
public static final int FULL_FLUSH = 3;
+/**
+ * Flush mode to use at the end of output. Can only be provided by the
+ * user by way of {@link #finish()}.
+ */
+private static final int FINISH = 4;
+
static {
ZipUtils.loadLibrary();
-
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Tue, Mar 27, 2018 at 10:10 AM, Alan Bateman wrote: > On 27/03/2018 00:09, David Lloyd wrote: >> I was going for some kind of consistency with the array variants (that >> is, name the parameter what it is), though they simply use "b" for >> their parameter name so that interpretation might be a stretch. >> Should I update them all? > > I think this would be good, if you don't mind doing it. OK. >> I'm not 100% familiar with the new JavaDoc categories (ok I'm 0% >> familiar), but the JEP for these says "This category consists of >> commentary, rationale, or examples pertaining to the API.". But this >> feels more like specification to me since it is "specifications that >> apply to all valid implementations, including preconditions, >> postconditions, etc.". Which is to say, if you don't provide enough >> remaining space in the output buffer, you will have incorrect >> operation as a result. WDYT? > > The current wording (which pre-dates your changes of course) reads more like > API advice. I think it's a bit confusing too as it doesn't define what a > "flush marker" is. I think we will need to re-word that sentence to make it > clearer. OK. I am at a loss for a better way to explain it though; any suggestions? >>> I don't have cycles just now to go through all the implementation but I >>> think Sherman is doing that. It will need careful review to avoid being >>> abused to attack memory outside of the buffer. I did check the use of >>> position() and limit() to calculate the remaining and these need correct. >> >> I hope this was a typo of "these seem correct" and not a typo of >> "these need correcting"? > > Oops, this was indeed a typo in my mail. OK great. -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On 27/03/2018 00:09, David Lloyd wrote: : I think they may have use in some cases e.g. reading a dictionary from a file, and generally in code which prefers byte buffers over arrays (completely outside of the direct/heap buffer question), and they were easy enough to implement. But I'm okay dropping these methods if that is what is desired. I'm in two minds on the setDictionary methods. No objection to including them for consistency reasons, I'm just not sure if they will really be used. : I also wonder if the parameter should be renamed to "input" or "source". I was going for some kind of consistency with the array variants (that is, name the parameter what it is), though they simply use "b" for their parameter name so that interpretation might be a stretch. Should I update them all? I think this would be good, if you don't mind doing it. : I'm not 100% familiar with the new JavaDoc categories (ok I'm 0% familiar), but the JEP for these says "This category consists of commentary, rationale, or examples pertaining to the API.". But this feels more like specification to me since it is "specifications that apply to all valid implementations, including preconditions, postconditions, etc.". Which is to say, if you don't provide enough remaining space in the output buffer, you will have incorrect operation as a result. WDYT? The current wording (which pre-dates your changes of course) reads more like API advice. I think it's a bit confusing too as it doesn't define what a "flush marker" is. I think we will need to re-word that sentence to make it clearer. I don't have cycles just now to go through all the implementation but I think Sherman is doing that. It will need careful review to avoid being abused to attack memory outside of the buffer. I did check the use of position() and limit() to calculate the remaining and these need correct. I hope this was a typo of "these seem correct" and not a typo of "these need correcting"? Oops, this was indeed a typo in my mail. -Alan
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Mon, Mar 26, 2018 at 9:53 AM, Alan Bateman wrote: > On 23/03/2018 19:17, David Lloyd wrote: >> >> All fixed. You're right, the catch blocks consolidated fairly easily >> and it looks better. I've attached the latest version of the patch. >> > I went through the APIs and javadoc changes attached to your last mail. > > This version addresses most of the points I brought up a few weeks ago and > the API generally looks good. The only methods that I'm not sure about is > the setDictionary variants as I don't see a bit need for these. I think they may have use in some cases e.g. reading a dictionary from a file, and generally in code which prefers byte buffers over arrays (completely outside of the direct/heap buffer question), and they were easy enough to implement. But I'm okay dropping these methods if that is what is desired. > Deflater.setInput currently has "The given buffer's position will be updated > ...". I think could be clearer by saying that the buffer position s > advanced by the deflate operations up to its limit. This will make it more > consistent with the wording in the deflate methods. OK. > I also wonder if the parameter should be renamed to "input" or "source". I was going for some kind of consistency with the array variants (that is, name the parameter what it is), though they simply use "b" for their parameter name so that interpretation might be a stretch. Should I update them all? > The deflate methods talk about "remaining space" which is a bit inconsistent > the buffer APIs where it uses "bytes remaining". I think we should try to > keep this as consistent as possible. OK. > Also the usage advice, "Make sure the buffer's remaining space ..." should > probably be moved to an @apiNote (this goes for the existing deflate methods > too). I'm not 100% familiar with the new JavaDoc categories (ok I'm 0% familiar), but the JEP for these says "This category consists of commentary, rationale, or examples pertaining to the API.". But this feels more like specification to me since it is "specifications that apply to all valid implementations, including preconditions, postconditions, etc.". Which is to say, if you don't provide enough remaining space in the output buffer, you will have incorrect operation as a result. WDYT? > I don't have cycles just now to go through all the implementation but I > think Sherman is doing that. It will need careful review to avoid being > abused to attack memory outside of the buffer. I did check the use of > position() and limit() to calculate the remaining and these need correct. I hope this was a typo of "these seem correct" and not a typo of "these need correcting"? > Style wide then we should try to keep thing consistent with the existing > code as possible (most of the "final" usages are a distraction and aren't > needed for example). OK I can remove them; it's been my personal convention for so many years that I don't see them when they're there (only when they're missing). But removing them here is OK with me. -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On 23/03/2018 19:17, David Lloyd wrote: All fixed. You're right, the catch blocks consolidated fairly easily and it looks better. I've attached the latest version of the patch. I went through the APIs and javadoc changes attached to your last mail. This version addresses most of the points I brought up a few weeks ago and the API generally looks good. The only methods that I'm not sure about is the setDictionary variants as I don't see a bit need for these. Deflater.setInput currently has "The given buffer's position will be updated ...". I think could be clearer by saying that the buffer position s advanced by the deflate operations up to its limit. This will make it more consistent with the wording in the deflate methods. I also wonder if the parameter should be renamed to "input" or "source". The deflate methods talk about "remaining space" which is a bit inconsistent the buffer APIs where it uses "bytes remaining". I think we should try to keep this as consistent as possible. Also the usage advice, "Make sure the buffer's remaining space ..." should probably be moved to an @apiNote (this goes for the existing deflate methods too). I don't have cycles just now to go through all the implementation but I think Sherman is doing that. It will need careful review to avoid being abused to attack memory outside of the buffer. I did check the use of position() and limit() to calculate the remaining and these need correct. Style wide then we should try to keep thing consistent with the existing code as possible (most of the "final" usages are a distraction and aren't needed for example). -Alan
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Fri, Mar 23, 2018 at 1:06 PM, Alan Bateman wrote: > Is there an up to date webrev on cr.openjdk.java.net that we can review? I'm an outside contributor so I don't have any easy way to prepare one; however, if you wish, you can look at the diff on my GitHub mirror here [1]. In case you're curious about the "version", it's the 16th local iteration for me, even though the updated patch I've just sent is only the 6th version sent to the list. [1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v16 -- - DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Fri, Mar 23, 2018 at 12:51 PM, Xueming Shen wrote:
>
> Hi David,
>
> Sorry for the late response. I will sponsor this change. Will prepare for
> the CSR for
> the new APIs.
Thanks!
> Couple more comments
>
> (1) Deflater.java/deflate(ByteBuffer, int);
> "In the case of FULL_FLUSH or SYNC_FLUSH ..."
> [...]
> (2) Inflater.java/setDictionary(ByteBuffer);
> "Sets the preset dictionary to the given array of bytes"
> --> bytes in the specified byte buffer?
>
> (3) Infalter.java.inflater(...)
> It appears there is opportunity to "consolidate" some of the repetitive code
> block,
> [...]
> for example the "catch (DFE e) { ...} the only difference is the
All fixed. You're right, the catch blocks consolidated fairly easily
and it looks better. I've attached the latest version of the patch.
--
- DML
commit 2c798586b3451ba9cfe3069e5ee2699caf2c38fb
Author: David M. Lloyd
Date: Fri Feb 16 11:00:10 2018 -0600
[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer
diff --git a/make/mapfiles/libzip/mapfile-vers
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
- Java_java_util_zip_Deflater_deflateBytes;
+ Java_java_util_zip_Deflater_deflateBytesBytes;
+ Java_java_util_zip_Deflater_deflateBytesBuffer;
+ Java_java_util_zip_Deflater_deflateBufferBytes;
+ Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+ Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
- Java_java_util_zip_Inflater_inflateBytes;
+ Java_java_util_zip_Inflater_inflateBytesBytes;
+ Java_java_util_zip_Inflater_inflateBytesBuffer;
+ Java_java_util_zip_Inflater_inflateBufferBytes;
+ Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+ Java_java_util_zip_Inflater_setDictionaryBuffer;
ZIP_Close;
ZIP_CRC32;
ZIP_FreeEntry;
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index c75dd4a33f0..a1720a78986 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -26,7 +26,13 @@
package java.util.zip;
import java.lang.ref.Cleaner.Cleanable;
+import java.lang.ref.Reference;
+import java.nio.ByteBuffer;
+import java.nio.ReadOnlyBufferException;
+import java.util.Objects;
+
import jdk.internal.ref.CleanerFactory;
+import sun.nio.ch.DirectBuffer;
/**
* This class provides support for general purpose compression using the
@@ -92,8 +98,9 @@ import jdk.internal.ref.CleanerFactory;
public class Deflater {
private final DeflaterZStreamRef zsRef;
-private byte[] buf = new byte[0];
-private int off, len;
+private ByteBuffer input = ZipUtils.defaultBuf;
+private byte[] inputArray;
+private int inputPos, inputLim;
private int level, strategy;
private boolean setParams;
private boolean finish, finished;
@@ -170,9 +177,14 @@ public class Deflater {
*/
public static final int FULL_FLUSH = 3;
+/**
+ * Flush mode to use at the end of output. Can only be provided by the
+ * user by way of {@link #finish()}.
+ */
+private static final int FINISH = 4;
+
static {
ZipUtils.loadLibrary();
-initIDs();
}
/**
@@ -216,16 +228,14 @@ public class Deflater {
* @see Deflater#needsInput
*/
public void setInput(byte[] b, int off, int len) {
-if (b== null) {
-throw new NullPointerException();
-}
if (off < 0 || len < 0 || off > b.length - len) {
throw new ArrayIndexOutOfBoundsException();
}
synchronized (zsRef) {
-this.buf = b;
-this.off = off;
-this.len = len;
+this.input = null;
+this.inputArray = b;
+this.inputPos = off;
+this.inputLim = off + len;
}
}
@@ -239,6 +249,31 @@ public class Deflater
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Is there an up to date webrev on cr.openjdk.java.net that we can review?
The inflate/deflate methods in the original proposal made sense, I'm
less sure about introducing a new setDictionary method.
-Alan
On 23/03/2018 17:51, Xueming Shen wrote:
Hi David,
Sorry for the late response. I will sponsor this change. Will prepare
for the CSR for
the new APIs.
Couple more comments
(1) Deflater.java/deflate(ByteBuffer, int);
"In the case of FULL_FLUSH or SYNC_FLUSH ..."
It's a copy/paste from the corresponding part in deflate(byte[], int,
int, int). The existing
one might not be clear. What it meant to say is "if the returned value
is equal to the
space available in the output buffer passed in ...then you need to
call it with more sapce,
greater than 6 bytes ...". So the {@code len} need to updated to
something like
{@code output.remainint()}.
(2) Inflater.java/setDictionary(ByteBuffer);
"Sets the preset dictionary to the given array of bytes"
--> bytes in the specified byte buffer?
(3) Infalter.java.inflater(...)
It appears there is opportunity to "consolidate" some of the
repetitive code block,
for example the "catch (DFE e) { ...} the only difference is the
"inputPos/input.position()"
line. Maybe we can move the try/catch out a "level" and consolidate
this catch block.
Sure it's a style preference. We can deal with it later.
Thanks,
Sherman
On 03/23/2018 08:18 AM, David Lloyd wrote:
Are there any further comments on this? If not, would it be possible
to get a sponsor for this change?
Sorry again for the detached email threads; I've learned my GMail
lesson well...
Thanks.
On Fri, Mar 16, 2018 at 8:25 AM, David Lloyd
wrote:
Sorry, that was an error on my part, caused by too much context
switching. I've posted an update at
https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v13 which is
also attached.
On Wed, Mar 14, 2018 at 8:53 PM, Xueming
Shen wrote:
Hi David,
https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12
Should we start to review the changes included in above link, or we
should
wait ? It appears
the API is being updated but some implementation have not been
updated to
follow the spec
yet, especially the piece that deals with the output
buffer/byteWritten when
DataFormatException
is raised, for example
(1) the "outputConsumedID" is defined but never used to update the
corresponding java field
in Inflater.c and
(2) the "outputConsumed" is used to update the output ByteBuffer
when DFE
raised (in Java), but
the corresponding "byteWritten" is not being updated before the
exception is thrown.
-Sherman
On 3/13/18, 10:46 AM, David Lloyd wrote:
Sorry all, it looks like GMail doesn't know how to keep replies with
the thread when you change the subject line. The follow-up to this
thread is
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
with only a few small changes as discussed above.
On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd
wrote:
On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd
wrote:
On Fri, Mar 2, 2018 at 12:49 PM, Xueming
Shen
wrote:
Hi David,
(1) Deflater.deflate(Bytebuffer)
the api doc regarding "no_flush" appears to be the
copy/paste of
the
byte[] version
without being updated to the corresponding ByteBuffer?
You're right, I missed that one. I've incorporated this fix
locally:
Oops, this should have been:
--- 8< --- cut here --- 8< ---
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..40f0d9736e2 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -481,9 +481,9 @@ public class Deflater {
* in order to determine if more input data is required.
*
*This method uses {@link #NO_FLUSH} as its compression
flush
mode.
- * An invocation of this method of the form {@code
deflater.deflate(b)}
+ * An invocation of this method of the form {@code
deflater.deflate(output)}
* yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
*
* @param output the buffer for the compressed data
* @return the actual number of bytes of compressed data
written to
the
--- 8< --- cut here --- 8< ---
--
- DML
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Hi David,
Sorry for the late response. I will sponsor this change. Will prepare for the
CSR for
the new APIs.
Couple more comments
(1) Deflater.java/deflate(ByteBuffer, int);
"In the case of FULL_FLUSH or SYNC_FLUSH ..."
It's a copy/paste from the corresponding part in deflate(byte[], int, int,
int). The existing
one might not be clear. What it meant to say is "if the returned value is equal
to the
space available in the output buffer passed in ...then you need to call it with
more sapce,
greater than 6 bytes ...". So the {@code len} need to updated to something like
{@code output.remainint()}.
(2) Inflater.java/setDictionary(ByteBuffer);
"Sets the preset dictionary to the given array of bytes"
--> bytes in the specified byte buffer?
(3) Infalter.java.inflater(...)
It appears there is opportunity to "consolidate" some of the repetitive code
block,
for example the "catch (DFE e) { ...} the only difference is the
"inputPos/input.position()"
line. Maybe we can move the try/catch out a "level" and consolidate this catch
block.
Sure it's a style preference. We can deal with it later.
Thanks,
Sherman
On 03/23/2018 08:18 AM, David Lloyd wrote:
Are there any further comments on this? If not, would it be possible
to get a sponsor for this change?
Sorry again for the detached email threads; I've learned my GMail lesson well...
Thanks.
On Fri, Mar 16, 2018 at 8:25 AM, David Lloyd wrote:
Sorry, that was an error on my part, caused by too much context
switching. I've posted an update at
https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v13 which is
also attached.
On Wed, Mar 14, 2018 at 8:53 PM, Xueming Shen wrote:
Hi David,
https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12
Should we start to review the changes included in above link, or we should
wait ? It appears
the API is being updated but some implementation have not been updated to
follow the spec
yet, especially the piece that deals with the output buffer/byteWritten when
DataFormatException
is raised, for example
(1) the "outputConsumedID" is defined but never used to update the
corresponding java field
in Inflater.c and
(2) the "outputConsumed" is used to update the output ByteBuffer when DFE
raised (in Java), but
the corresponding "byteWritten" is not being updated before the
exception is thrown.
-Sherman
On 3/13/18, 10:46 AM, David Lloyd wrote:
Sorry all, it looks like GMail doesn't know how to keep replies with
the thread when you change the subject line. The follow-up to this
thread is
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
with only a few small changes as discussed above.
On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd
wrote:
On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd
wrote:
On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen
wrote:
Hi David,
(1) Deflater.deflate(Bytebuffer)
the api doc regarding "no_flush" appears to be the copy/paste of
the
byte[] version
without being updated to the corresponding ByteBuffer?
You're right, I missed that one. I've incorporated this fix locally:
Oops, this should have been:
--- 8< --- cut here --- 8< ---
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..40f0d9736e2 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -481,9 +481,9 @@ public class Deflater {
* in order to determine if more input data is required.
*
*This method uses {@link #NO_FLUSH} as its compression flush
mode.
- * An invocation of this method of the form {@code
deflater.deflate(b)}
+ * An invocation of this method of the form {@code
deflater.deflate(output)}
* yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
*
* @param output the buffer for the compressed data
* @return the actual number of bytes of compressed data written to
the
--- 8< --- cut here --- 8< ---
--
- DML
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Are there any further comments on this? If not, would it be possible
to get a sponsor for this change?
Sorry again for the detached email threads; I've learned my GMail lesson well...
Thanks.
On Fri, Mar 16, 2018 at 8:25 AM, David Lloyd wrote:
> Sorry, that was an error on my part, caused by too much context
> switching. I've posted an update at
> https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v13 which is
> also attached.
>
> On Wed, Mar 14, 2018 at 8:53 PM, Xueming Shen wrote:
>> Hi David,
>>
>> https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12
>>
>> Should we start to review the changes included in above link, or we should
>> wait ? It appears
>> the API is being updated but some implementation have not been updated to
>> follow the spec
>> yet, especially the piece that deals with the output buffer/byteWritten when
>> DataFormatException
>> is raised, for example
>>
>> (1) the "outputConsumedID" is defined but never used to update the
>> corresponding java field
>> in Inflater.c and
>>
>> (2) the "outputConsumed" is used to update the output ByteBuffer when DFE
>> raised (in Java), but
>> the corresponding "byteWritten" is not being updated before the
>> exception is thrown.
>>
>> -Sherman
>>
>>
>>
>>
>> On 3/13/18, 10:46 AM, David Lloyd wrote:
>>>
>>> Sorry all, it looks like GMail doesn't know how to keep replies with
>>> the thread when you change the subject line. The follow-up to this
>>> thread is
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
>>> with only a few small changes as discussed above.
>>>
>>> On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd
>>> wrote:
On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd
wrote:
>
> On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen
> wrote:
>>
>> Hi David,
>>
>> (1) Deflater.deflate(Bytebuffer)
>> the api doc regarding "no_flush" appears to be the copy/paste of
>> the
>> byte[] version
>> without being updated to the corresponding ByteBuffer?
>
> You're right, I missed that one. I've incorporated this fix locally:
Oops, this should have been:
--- 8< --- cut here --- 8< ---
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..40f0d9736e2 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -481,9 +481,9 @@ public class Deflater {
* in order to determine if more input data is required.
*
*This method uses {@link #NO_FLUSH} as its compression flush
mode.
- * An invocation of this method of the form {@code
deflater.deflate(b)}
+ * An invocation of this method of the form {@code
deflater.deflate(output)}
* yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
*
* @param output the buffer for the compressed data
* @return the actual number of bytes of compressed data written to
the
--- 8< --- cut here --- 8< ---
--
- DML
>>>
>>>
>>>
>>
>
>
>
> --
> - DML
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Sorry, that was an error on my part, caused by too much context
switching. I've posted an update at
https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v13 which is
also attached.
On Wed, Mar 14, 2018 at 8:53 PM, Xueming Shen wrote:
> Hi David,
>
> https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12
>
> Should we start to review the changes included in above link, or we should
> wait ? It appears
> the API is being updated but some implementation have not been updated to
> follow the spec
> yet, especially the piece that deals with the output buffer/byteWritten when
> DataFormatException
> is raised, for example
>
> (1) the "outputConsumedID" is defined but never used to update the
> corresponding java field
> in Inflater.c and
>
> (2) the "outputConsumed" is used to update the output ByteBuffer when DFE
> raised (in Java), but
> the corresponding "byteWritten" is not being updated before the
> exception is thrown.
>
> -Sherman
>
>
>
>
> On 3/13/18, 10:46 AM, David Lloyd wrote:
>>
>> Sorry all, it looks like GMail doesn't know how to keep replies with
>> the thread when you change the subject line. The follow-up to this
>> thread is
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
>> with only a few small changes as discussed above.
>>
>> On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd
>> wrote:
>>>
>>> On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd
>>> wrote:
On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen
wrote:
>
> Hi David,
>
> (1) Deflater.deflate(Bytebuffer)
> the api doc regarding "no_flush" appears to be the copy/paste of
> the
> byte[] version
> without being updated to the corresponding ByteBuffer?
You're right, I missed that one. I've incorporated this fix locally:
>>>
>>> Oops, this should have been:
>>>
>>> --- 8< --- cut here --- 8< ---
>>>
>>> diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
>>> b/src/java.base/share/classes/java/util/zip/Deflater.java
>>> index 524125787a8..40f0d9736e2 100644
>>> --- a/src/java.base/share/classes/java/util/zip/Deflater.java
>>> +++ b/src/java.base/share/classes/java/util/zip/Deflater.java
>>> @@ -481,9 +481,9 @@ public class Deflater {
>>>* in order to determine if more input data is required.
>>>*
>>>*This method uses {@link #NO_FLUSH} as its compression flush
>>> mode.
>>> - * An invocation of this method of the form {@code
>>> deflater.deflate(b)}
>>> + * An invocation of this method of the form {@code
>>> deflater.deflate(output)}
>>>* yields the same result as the invocation of
>>> - * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
>>> + * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
>>>*
>>>* @param output the buffer for the compressed data
>>>* @return the actual number of bytes of compressed data written to
>>> the
>>>
>>> --- 8< --- cut here --- 8< ---
>>>
>>> --
>>> - DML
>>
>>
>>
>
--
- DML
commit ee5f766eeb64d5afa1f3ec1153b3184de35ff198
Author: David M. Lloyd
Date: Fri Feb 16 11:00:10 2018 -0600
[JDK-6341887] Update Inflater/Deflater to handle ByteBuffer
diff --git a/make/mapfiles/libzip/mapfile-vers
b/make/mapfiles/libzip/mapfile-vers
index d711d8e17f4..11ccc2d6ecb 100644
--- a/make/mapfiles/libzip/mapfile-vers
+++ b/make/mapfiles/libzip/mapfile-vers
@@ -33,20 +33,28 @@ SUNWprivate_1.1 {
Java_java_util_zip_CRC32_update;
Java_java_util_zip_CRC32_updateBytes0;
Java_java_util_zip_CRC32_updateByteBuffer0;
- Java_java_util_zip_Deflater_deflateBytes;
+ Java_java_util_zip_Deflater_deflateBytesBytes;
+ Java_java_util_zip_Deflater_deflateBytesBuffer;
+ Java_java_util_zip_Deflater_deflateBufferBytes;
+ Java_java_util_zip_Deflater_deflateBufferBuffer;
Java_java_util_zip_Deflater_end;
Java_java_util_zip_Deflater_getAdler;
Java_java_util_zip_Deflater_init;
Java_java_util_zip_Deflater_initIDs;
Java_java_util_zip_Deflater_reset;
Java_java_util_zip_Deflater_setDictionary;
+ Java_java_util_zip_Deflater_setDictionaryBuffer;
Java_java_util_zip_Inflater_end;
Java_java_util_zip_Inflater_getAdler;
- Java_java_util_zip_Inflater_inflateBytes;
+ Java_java_util_zip_Inflater_inflateBytesBytes;
+ Java_java_util_zip_Inflater_inflateBytesBuffer;
+ Java_java_util_zip_Inflater_inflateBufferBytes;
+ Java_java_util_zip_Inflater_inflateBufferBuffer;
Java_java_util_zip_Inflater_init;
Java_java_util_zip_Inflater_initIDs;
Java_java_util_zip_Inflater_reset;
Java_java_util_zip_Inflater_setDictionary;
+ Java_java_util_zip_Inflater_setDi
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Hi David,
https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v12
Should we start to review the changes included in above link, or we
should wait ? It appears
the API is being updated but some implementation have not been updated
to follow the spec
yet, especially the piece that deals with the output buffer/byteWritten
when DataFormatException
is raised, for example
(1) the "outputConsumedID" is defined but never used to update the
corresponding java field
in Inflater.c and
(2) the "outputConsumed" is used to update the output ByteBuffer when
DFE raised (in Java), but
the corresponding "byteWritten" is not being updated before the
exception is thrown.
-Sherman
On 3/13/18, 10:46 AM, David Lloyd wrote:
Sorry all, it looks like GMail doesn't know how to keep replies with
the thread when you change the subject line. The follow-up to this
thread is
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
with only a few small changes as discussed above.
On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd wrote:
On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd wrote:
On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen wrote:
Hi David,
(1) Deflater.deflate(Bytebuffer)
the api doc regarding "no_flush" appears to be the copy/paste of the
byte[] version
without being updated to the corresponding ByteBuffer?
You're right, I missed that one. I've incorporated this fix locally:
Oops, this should have been:
--- 8< --- cut here --- 8< ---
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..40f0d9736e2 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -481,9 +481,9 @@ public class Deflater {
* in order to determine if more input data is required.
*
*This method uses {@link #NO_FLUSH} as its compression flush mode.
- * An invocation of this method of the form {@code deflater.deflate(b)}
+ * An invocation of this method of the form {@code
deflater.deflate(output)}
* yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
*
* @param output the buffer for the compressed data
* @return the actual number of bytes of compressed data written to the
--- 8< --- cut here --- 8< ---
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Sorry all, it looks like GMail doesn't know how to keep replies with
the thread when you change the subject line. The follow-up to this
thread is
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-March/051960.html
with only a few small changes as discussed above.
On Fri, Mar 2, 2018 at 2:36 PM, David Lloyd wrote:
> On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd wrote:
>> On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen
>> wrote:
>>> Hi David,
>>>
>>> (1) Deflater.deflate(Bytebuffer)
>>> the api doc regarding "no_flush" appears to be the copy/paste of the
>>> byte[] version
>>> without being updated to the corresponding ByteBuffer?
>>
>> You're right, I missed that one. I've incorporated this fix locally:
>
> Oops, this should have been:
>
> --- 8< --- cut here --- 8< ---
>
> diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
> b/src/java.base/share/classes/java/util/zip/Deflater.java
> index 524125787a8..40f0d9736e2 100644
> --- a/src/java.base/share/classes/java/util/zip/Deflater.java
> +++ b/src/java.base/share/classes/java/util/zip/Deflater.java
> @@ -481,9 +481,9 @@ public class Deflater {
> * in order to determine if more input data is required.
> *
> * This method uses {@link #NO_FLUSH} as its compression flush mode.
> - * An invocation of this method of the form {@code deflater.deflate(b)}
> + * An invocation of this method of the form {@code
> deflater.deflate(output)}
> * yields the same result as the invocation of
> - * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
> + * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
> *
> * @param output the buffer for the compressed data
> * @return the actual number of bytes of compressed data written to the
>
> --- 8< --- cut here --- 8< ---
>
> --
> - DML
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Fri, Mar 2, 2018 at 2:34 PM, David Lloyd wrote:
> On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen wrote:
>> Hi David,
>>
>> (1) Deflater.deflate(Bytebuffer)
>> the api doc regarding "no_flush" appears to be the copy/paste of the
>> byte[] version
>> without being updated to the corresponding ByteBuffer?
>
> You're right, I missed that one. I've incorporated this fix locally:
Oops, this should have been:
--- 8< --- cut here --- 8< ---
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..40f0d9736e2 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -481,9 +481,9 @@ public class Deflater {
* in order to determine if more input data is required.
*
* This method uses {@link #NO_FLUSH} as its compression flush mode.
- * An invocation of this method of the form {@code deflater.deflate(b)}
+ * An invocation of this method of the form {@code
deflater.deflate(output)}
* yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
*
* @param output the buffer for the compressed data
* @return the actual number of bytes of compressed data written to the
--- 8< --- cut here --- 8< ---
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen wrote:
> Hi David,
>
> (1) Deflater.deflate(Bytebuffer)
> the api doc regarding "no_flush" appears to be the copy/paste of the
> byte[] version
> without being updated to the corresponding ByteBuffer?
You're right, I missed that one. I've incorporated this fix locally:
--- 8< --- cut here --- 8< ---
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..9cf735a9477 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -483,7 +483,7 @@ public class Deflater {
* This method uses {@link #NO_FLUSH} as its compression flush mode.
* An invocation of this method of the form {@code deflater.deflate(b)}
* yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
*
* @param output the buffer for the compressed data
* @return the actual number of bytes of compressed data written to the
--- 8< --- cut here --- 8< ---
I'll post an amended patch once we work out the other points below
(and any other feedback that comes in the meantime).
> (2) Inflater.inflate()
> a "inputConsumed" field is added to handle the "bytes-read" if a DFE is
> being thrown.
>
> While the API doc specifies that "if the setInput(bytebuffer) method
> was call "
> it appears the inputPos/bytesRead are being updated for the
> "setInput(byte[]) case
> as well. This might be a desirable change, but is an "incompatible"
> behavior change
> as well. I doubt if there is any existing app depending on this
> existing behavior though,
> it's really hard, if not impossible, to try to recover from this kind
> of error
I agree; this was implemented based on Alan's points but I could go
either way with it.
I had a thought that I could update the position only in the buffer
input case, not the array input case, but this would still have the
effect of changing the behavior of Inflater.getRemaining() when the
input is a buffer, which is still technically an incompatibility. So
the three options are:
* Always update the input position on exception
* Only update the input position on exception when the input is a ByteBuffer
* Never update the input position on exception
AFAICT there are no inherent technical issues with any of these
approaches beyond the obscure compatibility change. Since the
behavior was previously unspecified, I think that decreases the
compatibility risk even further, so I'm inclined to think that #1 is
the best overall choice.
> There might also bring in a little inconsistency, as we are updating
> the "input" in this
> case, but why not the "output" buffer? It's true there is no way to do
> that with the
> inflate(byte[] ...), it's kinda doable for the inflate(ByteBuffer) and
> in fact there might
> be bytes that have been written into the output ByteBuffer in this
> case.
Since there is no way to get the output bytes consumed in the array
case, this is not an incompatible change, so I am happy to include it
in my next revision. However I think it only makes sense to do this
if it is consistent with input; i.e. if option #3 above is chosen then
it would be inconsistent to update the output position on exception
and we should not do it.
> (3) there are usages like
> Math.max(buf.limit() - outputPos, 0);
> just wonder if there is any reason not using existing api method.
> Math.max(buf.remaining(), 0);
This is because the buffer's position can change between the call to
buf.position() and buf.remaining(), which can have the ultimate effect
of allowing invalid memory accesses. By acquiring the position and
limit each one time, I can guarantee some consistent view of these
properties. This is consistent with (for example) sun.nio.ch.IOUtil.
Also just as a general principle, buf.remaining() reads both the limit
and position, and since I already have the position, it makes sense to
just read the limit which is the only other piece of data that must be
read in order to correctly calculate these values.
Thanks for the feedback.
--
- DML
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
Hi David, (1) Deflater.deflate(Bytebuffer) the api doc regarding "no_flush" appears to be the copy/paste of the byte[] version without being updated to the corresponding ByteBuffer? (2) Inflater.inflate() a "inputConsumed" field is added to handle the "bytes-read" if a DFE is being thrown. While the API doc specifies that "if the setInput(bytebuffer) method was call " it appears the inputPos/bytesRead are being updated for the "setInput(byte[]) case as well. This might be a desirable change, but is an "incompatible" behavior change as well. I doubt if there is any existing app depending on this existing behavior though, it's really hard, if not impossible, to try to recover from this kind of error There might also bring in a little inconsistency, as we are updating the "input" in this case, but why not the "output" buffer? It's true there is no way to do that with the inflate(byte[] ...), it's kinda doable for the inflate(ByteBuffer) and in fact there might be bytes that have been written into the output ByteBuffer in this case. (3) there are usages like Math.max(buf.limit() - outputPos, 0); just wonder if there is any reason not using existing api method. Math.max(buf.remaining(), 0); sherman On 03/02/2018 08:07 AM, David Lloyd wrote: The third... and hopefully final... version of this patch is attached. It is the same as V2, however it also uses Reference.reachabilityFence() defensively on buffers. This may be necessary because there are code paths which may allow such buffers to be GCed after their address is acquired but before the native code successfully is able to read it. The online version of this patch is visible at [1]. [1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v8
Re: [JDK-6341887] RFR: Patch V3: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
> On Mar 2, 2018, at 8:07 AM, David Lloyd wrote: > > The third... and hopefully final... version of this patch is attached. > It is the same as V2, however it also uses > Reference.reachabilityFence() defensively on buffers. This may be > necessary because there are code paths which may allow such buffers to > be GCed after their address is acquired but before the native code > successfully is able to read it. > > The online version of this patch is visible at [1]. > I have not been tracking this patch closely, but the use of Reference.reachabilityFence looks good to me. Paul. > [1] https://github.com/dmlloyd/openjdk/commit/zlib-bytebuffer-v8 > > -- > - DML >
