RFR: 8318756 Create better internal buffer for AEADs [v3]

Anthony Scarpino ascarpino at openjdk.org
Wed Dec 6 02:45:34 UTC 2023


On Mon, 4 Dec 2023 07:27:14 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:

>> As I understand the `ByteArrayOutputStream` code, the `ArraysSupport.newLength()` will double the allocation each time.  So if the buffer is 1k size and it wants to add one more byte, the method will allocate 2k.
>> I agree that in my change, if you send one byte at a time, it will be doing a lot of allocating.  But I don't believe that is the general case.  If you have examples of apps doing small allocations, let me know and I can come up with a different scheme.  But at this point I thought it was a bitter risk more allocations in the less-likely situation, than unused allocation in what I see as a more common case.
>
> Well, as stated above, any application using CipherInputStream will do O(N) reallocations here, bringing back [JDK-8298249](https://bugs.openjdk.org/browse/JDK-8298249); you might want to check with the reporter if this actually affects any real applications.
> 
> For reference, the results with this patch:
> 
> Benchmark                         (dataSize)  (keyLength)  (provider)   Mode  Cnt       Score      Error  Units
> AESGCMCipherOutputStream.decrypt       16384          128              thrpt   40   30325.669 ? 1616.428  ops/s
> AESGCMCipherOutputStream.decrypt     1048576          128              thrpt   40       7.034 ?    0.479  ops/s
> 
> Baseline:
> 
> Benchmark                         (dataSize)  (keyLength)  (provider)   Mode  Cnt       Score      Error  Units
> AESGCMCipherOutputStream.decrypt       16384          128              thrpt   40   52171.497 ? 6229.841  ops/s
> AESGCMCipherOutputStream.decrypt     1048576          128              thrpt   40     470.844 ?  179.817  ops/s
> 
> i.e. with the patch, decryption is 40% slower for 16KB stream, 98% (or 50x) slower for 1MB stream.

Ok.. I updated the code to handle it more like it did.  With one update(), the memory usage is still how I intended.  So keeping the CipherOutputStream case, where it's multiple update() calls, it's a problem.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16487#discussion_r1416585796



More information about the security-dev mailing list