Circled around on this issue tonight and tried to look into the mysterious behavior with the value of MAX_CHUNKS (the constant that determines how many checksum chunks worth we'll read in a single go)
I wrote a quick benchmark which read a 1GB file out of /dev/shm with checksums using different values of MAX_CHUNKS. For each value, I ran 50 separate trials and calculated the average, as well as doing t-tests to figure out which results were within noise of each other.
../hadoop-3205-bench/mc_32.user 3.1156 ** everything below here is within noise
../hadoop-3205-bench/mc_32.elapsed 3.4392 ** everything below here is within noise
These were all done with a 64KB io.file.buffer.size, which would make us expect an optimal value of 128, since it should eliminate a copy. The results show that there are no gains to be had after 16 or 32 chunks being read at a time (8-16KB). The L1 cache on this machine is 128K, so that's not the magic number either.
So basically, the performance improvement here remains a mystery to me, but it's clear there is one - about 13% for reading out of RAM on the machine above. Given these results, I'd propose hard coding MAX_CHUNKS to 32 rather than basing it on io.file.buffer.size as I earlier figured.
On a separate note, some review responses:
Was this way before your change but the back-to-back if statements on line 252-253 could be combined triviallly.
I think you missed the "chunkPos += read;" outside the inner if? Java seems to occasionally return -1 for EOF for some reason so I was nervous about letting that happen outside the if. I'd be happy to add an assert read >= 0 though for this case and make it part of the contract of readChunks to never return negative.
The rest of the review makes sense, and I'll address those things and upload a new patch.