dao-jun commented on PR #4741: URL: https://github.com/apache/bookkeeper/pull/4741#issuecomment-4522269261
@hangc0276 Thanks for your professional review! When working on this PR, I definitely took this issue into consideration. Although `readEntrySize` only needs to read 4 bytes of data, during actual I/O operations the disk may read more data – typically 4 KB. If the page cache is hit, no additional disk I/O is required. Compared to the original implementation, this does save disk I/O. For Direct I/O, this change may also waste a buffer block (or not, if the subsequent request is a tail read). In that case, only memory allocation and other similar overheads are saved. In short, at the time I believed this change would indeed reduce system overhead – at the very least, it would save on memory allocation and similar costs. I understand your concern: introducing such a major change might only reduce some non‑I/O overhead (in the worst case, for example, when the entry size is very small). In order not to block the release of version 4.18, I have decided to temporarily apply the solution you mentioned in your comment. However, because Bookkeeper is widely used in Pulsar, I think the entry size tends to be uneven and relatively large. On the client side, a batch is typically 128 KB, but to improve throughput it is often set larger (though not exceeding 5 MB). I currently don't have time to provide benchmark data to quantify the performance improvement of this PR, but I may continue that work in the future. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
