facebook / rocksdb

A library that provides an embeddable, persistent key-value store for fast storage.
http://rocksdb.org
GNU General Public License v2.0
28.71k stars 6.34k forks source link

Explain why RandomAccessFileReader* is not passed into FilePrefetchBuffer constructor #13159

Open archang19 opened 5 hours ago

archang19 commented 5 hours ago

Summary

In https://github.com/facebook/rocksdb/pull/13118#discussion_r1842848359, we decided to make a separate follow-up PR that refactors FilePrefetchBuffer to determine use_fs_buffer once at construction time.

The change would have involved passing in the RandomAccessFileReader* directly to the constructor, and using that to determine use_fs_buffer. This would avoid repeatedly calling UseFSBuffer(RandomAccessFileReader* reader) during the actual prefetch requests.

I started working on this refactoring change but ran into issues with these 2 files, which used GetOrCreatePrefetchBuffer

As I explained in the added code comments, sometimes the RandomAccessFileReader* is not available when we construct the FilePrefetchBuffer, so although it is not the most elegant, I think right now it makes sense to pass in the reader into the Prefetch / PrefetchAsync / TryReadFromCache calls. Maybe there is a workaround but I don't think the refactor would be worth it.

Test Plan

N/A (comments)

facebook-github-bot commented 5 hours ago

@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.