apache
IO-649 - Improve the performance of the contentEquals() methods.
This change modifies the contentEquals() methods to internally buffer content into a byte/char array and to then do batch comparisons of those arrays using Arrays.equals instead of using a BufferedInputStream or BufferedReader and making use of the single byte/char read() methods.
This reduces the number of method invocations by a factor equal to the buffer size and avoids casting every byte read to an int and improves performance significantly.
The following table shows the performance increase over 1000 iterations of comparing 2 1GB InputStream of binary data (stored in memory to avoid I/O). This test was performed on an EC2 M4.4XL host using Java 1.8.0.232 and there was a forced System.gc() between each iteration to avoid GC as a source of latency:
Average: 7236 to 858ms (8.43x speedup) P50: 7224 to 856ms (8.44x speedup) P90: 7249 to 860ms (8.43x speedup) P99: 7410 to 913ms (8.12x speedup) P100: 8330 to 1278ms (6.52x speedup)
The following table shows the performance increase over 1000 iterations of comparing 2 1GB Reader of character data (stored in memory to avoid I/O). This test was performed on an EC2 M4.4XL host using Java 1.8.0.232 and there was a forced System.gc() between each iteration to avoid GC as a source of latency:
Average: 11281 to 1737ms (6.50x speedup) P50: 11262 to 1735ms (6.49x speedup) P90: 11292 to 1741ms (6.49x speedup) P99: 11707 to 1774ms (6.60x speedup) P100: 12176 to 1884ms (6.46x speedup)
Coverage increased (+0.06%) to 89.837% when pulling 2a55f16dce4476bbeae57739fd03421cf4edc970 on brettlounsbury:master into 133fb1717443f3110d812dd65a5238a790ce39b1 on apache:master.
I took a look at the coverage drop. I forgot to add tests that exercise the bounds checking of the bufferSize inputs to both contentEquals method. I've pushed a new version with that.
Hi @brettlounsbury.
Thank you for your PR and updates. I've sprinkled comments throughout.
Performance claims must be backed up somehow, please add a JMH benchmark to the test so myself and others can compare mileage.
For examples of JMH usage, please see other Apache Commons components like Commons BCEL, Commons Crypto, Commons CSV, Commons Lang, Commons Math, and Commons RNG.
Thank you! Gary
Hi @garydgregory,
I don't see any comments from you other than the request for a benchmark.
I have updated IOUtils to include a test dependency on JMH and and to include a benchmark that shows BufferedInputStream.read() performance compared to InputStream.read(byte[]) performance. This test creates an 8MB file on disk once before the benchmark is ran and reads it back in. It then just keeps a running sum of the bytes in the file (and ignores any overflow/underflow) and blackhole's the sum at the end to avoid any compiler optimization.
Results look something like this (Multi-byte reads are about 3.5x faster): Benchmark Mode Cnt Score Error Units SingleByteReadVsMultiByteReadPerformanceTest.testReadMultiBytes avgt 25 6220618.290 ± 204562.812 ns/op SingleByteReadVsMultiByteReadPerformanceTest.testReadSingleByte avgt 25 21740409.146 ± 711294.924 ns/op
@garydgregory,
I really appreciate your thorough review of my code. I think at this point I have addressed your comments, but if you have any others please let me know.
I will add that I made the IOUtils.DEFAULT_BUFFER_SIZE constant package private (default visibility) so it could be referenced from IOUtils tests so that I can ensure the logic makes sense around buffer boundaries and I didn't want to clone that property since it could later be mutated and invalidate some of the edge condition testing being done.
@garydgregory - Any other feedback here or anything else I can do to improve this?
@garydgregory - Can you take another look or suggest another reviewer?
Sorry about the delay, last day of vacation here ;-)
No worries - hope you had a great vacation!
@garydgregory - I think all necessary changes have been made. Are you able to take a look?
@brettlounsbury Please rebase on master and keep your changes to a minimum, the current change set is for 53 files! Also compare with https://issues.apache.org/jira/projects/IO/issues/IO-670