[ntuple] Save a memcpy when reading a compressed Anchor in RMiniFile #18612
Conversation
Do: (read into unzipBuf) -> (unzip into finalBuf) instead of: (read into finalBuf) -> (unzip into unzipBuf) -> (copy back to finalBuf)
Test Results 19 files 19 suites 4d 12h 9m 36s ⏱️ Results for commit b81ea42. ♻️ This comment has been updated with latest results. |
| // Unzip into the final buffer | ||
| RNTupleDecompressor::Unzip(unzipBuf.get(), objNbytes, key.fObjLen, ntuple); | ||
| } else { | ||
| ReadBuffer(ntuple, objNbytes, offset); |
There was a problem hiding this comment.
I don't really think this is on the critical path, and now we have two calls to ReadBuffer...
There was a problem hiding this comment.
What do you mean two calls? They're mutually exclusive so we only have one; we're simply doing one less operation in one of the branches.
There was a problem hiding this comment.
There are two lines of code where we call ReadBuffer, which we will have to make sure updated in sync. That current code is more maintainable.
There was a problem hiding this comment.
Why is ReadBuffer harder to maintain than memcpy? That too needs to be synchronized, and both are trivial things to do
There was a problem hiding this comment.
Skipping a useless mempcy can be a significant performance difference, for example in case of tight looping over a single branch and is worth the slight extra complexity.
There was a problem hiding this comment.
This code is about the anchor which is read once when opening the file. There is simply no performance implication in that.
There was a problem hiding this comment.
which btw also means that the amount of memory copied is 78 bytes...
There was a problem hiding this comment.
I missed that ... indeed for such a low size ... even doing the compression is debatable ....
There was a problem hiding this comment.
For RMiniFile, this was explicitly implemented in 7b03bcc. I believe TFile always attempts compression and goes with it if beneficial, ie compressed size smaller than uncompressed.
There was a problem hiding this comment.
This change is not about performance at all, it's about code simplicity and straightforwardness. This code does less work and therefore is simpler to understand. However if I'm the only one who thinks so, this PR can be closed and not much of value would be lost, being such a small change.
Do:
(read into unzipBuf) -> (unzip into finalBuf)
instead of:
(read into finalBuf) -> (unzip into unzipBuf) -> (copy back to finalBuf)
Checklist: