RFR: 8299748: java/util/zip/Deinflate.java failing on s390x [v2]

[Jaikiran Pai]

On Thu, 2 Feb 2023 08:27:55 GMT, Amit Kumar <amitkumar at 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