[JDK-6341887] Patch: java.util.zip: Add ByteBuffer methods to Inflater/Deflater
david.lloyd at redhat.com
Mon Feb 19 15:49:01 UTC 2018
On Sat, Feb 17, 2018 at 3:18 PM, Xueming Shen <xueming.shen at oracle.com> wrote:
> Hi David,
> Thanks for taking this one :-) some comments here.
Thanks for the review!
> (1) I would assume you might have to do more for ByteBuffer, something like
> btw, any reason go unsafe to get the byte? instead of
Yes: input should always be settable from a read-only heap buffer
without a performance penalty (i.e. copying the contents). This btw
is why there is also an explicit check for read-only later on.
> (2) It might be a concerned that you have to warp the input byte every time
> the inflate/deflate(byte...) is called. (Yes, I'm constantly hearing people
> complain that you have to wrap the byte to ByteBuffer to use CharsetEn/
> Decoder, or even the implementation detail inside StringCoder), and it might
> be taken as a "regression". So it might be desired to wire the "bf.hasArray())
> path inside de/encode(ByteArray) back to de/encode(byte, int, int), to avoid
> the "unnecessary" ByteBuffer wrap.
I can do some testing to see if there is an impact. I am working on
the basis that the wrap may be optimized away (as it is in certain
similar cases), but Inflater/Deflater is not final (nor are the
inflate/deflate methods) that might not be true in practice.
> (3) Same "wrap concern" argument might go to the setInput(bye ...) as well.
> I'm not sure if it is worth keeping both byte/int/int and ByteBuffer as the
> "input" field of In/Deflater.
Since input is stored on the object, the above theoretical
optimization is much less likely. Again I can do some testing; it
might be a good idea to have separate byte/int/int and ByteBuffer in
the end, though the added expense of checking for and updating the
ByteBuffer position after each call in addition to the byte/int/int
might nullify the benefit. Testing is required...
> (4) assume we keep the "wrap" approach. It appears ByteBuffer.wrap() does
> check the byte/off/len and throw an IndexOutOfBoundsException. So it might
> be better to take advantage of that check.
I wanted to however it throws the wrong exception type; I could catch
and rethrow I guess though if you think that is better (assuming we
keep the "wrap" approach as you say).
> (5) Deflater.input need to be initialized to a non-null value.
> Btw ZipUtil.defaultBuf needs to be "direct"?
It doesn't need to be, but the direct buffer code path is possibly
somewhat "friendlier" to GC since there's no GetPrimitiveArrayCritical
> (6) It might be desired to have some jmh measure to make sure byte case
> does not have regression.
I'll work on this when I get a chance (maybe not until Friday though).
One other thought I had this weekend was that I could possibly improve
things by getting the direct buffer address on the Java side, and pass
it in to JNI to avoid the calls to GetDirectBufferAddress. Another
thought was to eliminate the remaining SetBooleanField calls by using
the remaining two bits in the doInflate/doDeflate methods' return
values. I'm not sure how expensive these calls are though. I could
also replace the JNI field reads with more method parameters if this
is a valuable thing to do.
More information about the core-libs-dev