Add buffer prefetch support for non directIO usecase #7312
Conversation
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
jay-zhuang
changed the title
include/rocksdb/file_system.h
Outdated
| // an internal buffer `FilePrefetchBuffer` will be used for prefetch. | ||
| // For a file system that doesn't support prefetch, overriding it to false can | ||
| // improve the scan performance by leveraging the internal prefetch buffer. | ||
| virtual bool SupportPrefetch() const { return true; } |
There was a problem hiding this comment.
This creates a dependency on the file system implementation to override and return false. We could make Prefetch() default return NotSupported. Any implementation supporting prefetch must already be overriding it, so as long as RocksDB handles the return status of Prefetch we should be fine.
Alternatively, SupportPrefetch could return false by default, and we can override in implementations that support Prefetch (which is basically just PosixRandomAccessFile).
There was a problem hiding this comment.
I was concerned about changing the default behavior of Prefetch(). For example, if a user defined their own FS:
- if
Prefetch()is not implemented by the user, then we will still callPrefetch()which does nothing. - if
Prefetch()is implemented by the user likePosixRAF, then we will just call that.
Now if we change the default Prefetch() to NotSupported, then for the usecase 1., the behavior is going to be changed to use FilePrefetchBuffer. Which maybe a good thing for the most of the users, but I'm not sure if it's okay to make such default behavior change. What do you think?
There was a problem hiding this comment.
If we want to retain the behavior of usecase 1, another option is to require the FS implementation to override Prefetch to return NotSupported. Granted, it requires the FS to override it and thus introduces a dependency, but it feels a little odd to have SupportPrefetch as a separate interface.
But going one step further, I think its ok to change the default behavior. Except in the case of iterator, we prefetch data when we know we're going to need it, so the possibility of reading throwaway data is minimal. In the case of iterator, we use a user specified readahead_size or slowly increase the readahead size so it usually only applies to long range scans. Either way, no harm done.
Most users tend to just use one of the provided file systems, and are not aware of these nuances. Its unlikely that they've consciously opted in to the default behavior.
There was a problem hiding this comment.
Sounds good. I agree that it will make the interface clean.
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| ASSERT_EQ(0, buff_prefetch_count); | ||
| } else { | ||
| ASSERT_FALSE(fs->IsPrefetchCalled()); | ||
| } |
There was a problem hiding this comment.
Should we check buff_prefetch_count > 0 here too?
|
@jay-zhuang The PR description also needs to be updated, since the reference to |
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@jay-zhuang merged this pull request in c2485f2. |
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook/rocksdb#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook/rocksdb#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook/rocksdb#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook/rocksdb#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook/rocksdb#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook/rocksdb#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527 Signed-off-by: Changlong Chen <levisonchen@live.cn>
Summary: A new file interface `SupportPrefetch()` is added. When the user overrides it to `false`, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support. Pull Request resolved: facebook/rocksdb#7312 Reviewed By: anand1976 Differential Revision: D23329847 Pulled By: jay-zhuang fbshipit-source-id: 71cd4ce6f4a820840294e4e6aec111ab76175527 Signed-off-by: Changlong Chen <levisonchen@live.cn>
A new file interface
SupportPrefetch()is added. When the user overrides it tofalse, an internal prefetch buffer will be used for readahead. Useful for non-directIO but FS doesn't have readahead support.