Add BlobFetcherCopyReadOptions #11236
Conversation
db/blob/blob_fetcher.h
Outdated
| }; | ||
|
|
||
| class BlobFetcherCopyReadOptions : public BlobFetcher { |
There was a problem hiding this comment.
This naming BlobFetcherCopyReadOptions sounds like it's a type of ReadOptions than BlobFetcher. How about BlobFetcherWithReadOptionsCopy.
db/blob/blob_fetcher.h
Outdated
| }; | ||
|
|
||
| class BlobFetcherCopyReadOptions : public BlobFetcher { | ||
| const ReadOptions read_options_copy_; |
There was a problem hiding this comment.
nit suggestion: how about moving the data members to be after the function members and set its private section explicitly?
There was a problem hiding this comment.
nit suggestion: how about moving the data members to be after the function members and set its private section explicitly?
Done!
jowlyzhang
force-pushed
the
BlobFetcherReadOptionsUseRef
branch
from
3e773da
to
6598c8a
Compare
| class BlobFetcherWithReadOptionsCopy : public BlobFetcher { | ||
| public: | ||
| BlobFetcherWithReadOptionsCopy(const Version* v, const ReadOptions& ro) | ||
| : BlobFetcher(v, read_options_copy_), read_options_copy_(ro) {} |
There was a problem hiding this comment.
If BlobFetcher uses this reference in its constructor, it will cause some hard to find bugs. This subclass is enforcing an unusual requirement on its base class. Future users of BlobFetcher needs to be conscious of the scope of their ReadOptions relative to their BlobFetcher in order to choose the right one of the two. This at least requires more documentation of this class. I wonder if the copying saved is worth introducing this pattern. Checking with @ltamasi to make a call here.
There was a problem hiding this comment.
Thanks @jowlyzhang ! I agree that reference members are pretty accident prone: as an example, if BlobFetcher ever started to use read_options_ in its destructor, BlobFetcherWithReadOptionsCopy would break since the ReadOptions in the derived class would have been destroyed by the time the base class destructor runs.
@rockeet The goal of the PR is to save the ReadOptions copy that happens in Version::Get and Version::MultiGet, right? We could achieve that by changing GetContext so it takes a back pointer to Version (instead of a BlobFetcher pointer) and calls Version::GetBlob directly.
There was a problem hiding this comment.
We could achieve that by changing
GetContextso it takes a back pointer toVersion(instead of aBlobFetcherpointer) and callsVersion::GetBlobdirectly.
Hm, actually, scratch that - the key issue is how we're going to access the ReadOptions struct since GetContext doesn't have access to it either. Yeah, saving this copy might not be worth the extra complexity then.
BlobFetcherhas a copy ofReadOptionswhich waste CPU.This PR optimized this copy of
ReadOptionsto a reference, also addBlobFetcherCopyReadOptionsfor long lived BlobFetcher object.