On Thu, 2 Feb 2023 08:27:55 GMT, Amit Kumar <amitku...@openjdk.org> wrote:
>> DeInflate.java test fails on s390x platform because size for out1 array
>> which is responsible for storing the compressed data is insufficient. And
>> being unable to write whole compressed data on array, on s390 whole data
>> can't be recovered after compression. So this fix increase Array size (for
>> s390).
>
> Amit Kumar has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change acc to Alan comments
Thank you Amit for running this test. So the custom zlib implementation does
indeed returns false for `needsInput()`, which is a good thing.
Based on the tests so far, what this suggests to me is that the `deflate`
implementation of this custom `zlib` library compresses the data to a size
larger than what this test expects. It isn't clear how large it is going to be
or whether it will always be the same size after deflate, based on what you
note:
> But I guess this behaviour could be explained (by zEDC). On s390x, there is
> additional instruction present and it could be enabled by setting DFLTCC=1.
> And for accelerator the size of compressed data could go2 times the size of
> actual data. Again this is not deterministic as well, i.e. for same data
> there could be different valid deflate stream.
This specific test `DeInflate` notes that it's there for basic deflate and
inflate testing. As far as I can see, there's nothing in the specification of
`Deflater#deflate()` which states that the current size of the output buffer
that the test uses is mandated/expected by spec:
> byte[] dataOut1 = new byte[dataIn.length + 1024];
So `1024` is a reasonable extra length that the test has been using
successfully so far. It appears that this is not enough on s390x with this
custom implementation of zlib.
I believe it doesn't violate any spec. So your proposed change (which Alan
recommended) to use a `ByteArrayOutputStream` to keep accumulating the deflated
(and later inflated) data, until the deflater is `finished()` sounds fine to
me. The important thing here is that the inflated content from this deflated
content matches the original input. From what I see in this discussion, with
these changes, it does indeed match and thus the test passes. So I think that's
a good thing.
Now coming to this proposed patch, now that you are using local
ByteArrayOutputStream(s) for the deflate/inflate part in this `check()` method,
I think the `out1` and `out2` should no longer be needed in this method and
thus the method signature can be changed to remove these params. I see that
this is the only place where this method is used, so changing the method
signature of `check()` should be OK and shouldn't impact other parts of the
test.
While we are at it, just for the sake of testing, undo the changes I suggested
in my last reply and use the current PR's code (afresh) and print out the
length of the final deflated content, something like:
out1 = baos.toByteArray();
System.out.println("Deflated length is: " + out1.length + " for input of
length: " + len);
I'm curious how large the deflated content would be.
Finally, are you or someone in your team, in contact with the author(s) of the
custom zlib implementation
https://github.com/iii-i/zlib/commit/113203437eda67261848b14b6c80a33ff7e33d34?
Would you be able to ask for their inputs on whether this (large?) difference
in the deflated content size expected and are there any specific input (sizes)
that this shows up or is it for all inputs?
Finally, I would wait to hear from Alan or Lance on whether these changes are
OK, given the results of the experiments we have seen so far.
-------------
PR: https://git.openjdk.org/jdk/pull/12283
