Make types of Immutable/Mutable Options fields match that of the underlying Option #8176
Conversation
Changed (almost all) the values in the Immutable option formats to match the Options form, meaning that more are shared_ptr. This change will allow an Option to be re-created from its Immutable and Mutable forms. This is stage 1 of this process. A goal is to introduce a ImmutableOptions class that contains the Immutable DB and CF Options to eliminate cross referencing between the two (there are options in ImmutableCFOptions that are ImmutableDBOptions. Because of the linkage, promoting options to Mutable can be difficult).
To test performance delta.
|
I'm not sure this "fixes" anything as DB should generally outlive everything. The case for Statisitics outliving the DB (#8167) I believe is exceptional. |
I agree that the Statistics/Logger should not outlive the DB. Adding a shared_ptr here guarantees they do not outlive this object. I agree that the extra reference should not be necessary, but is little cost to prevent that risk. Is there a scenario in which this does not solve an existing (or potential) lifetime problem? |
|
Arguably, thanks to ASAN it's easier for us to catch use-after-free bugs than it is for us to catch bugs of excessive object lifetime or excessive CPU usage (under specific conditions). So I'm still not convinced that copying shared_ptr when a raw pointer copy should suffice (architecturally) is actually "safer." |
Can ImmutableCFOptions have a pointer back to the parent ImmutableDBOptions to solve these concerns? Or are we concerned about a level of indirection to get to |
I am not certain which concerns you are referring to. If we maintained a pointer to the ImmutableDBOptions inside the ImmutableCFOptions, we would need to pass in a long-lived pointer (the address in the DBImpl) rather than what we are doing now. This change would only effectively save some smart pointer references/increases as ImmutableCFOptions are created (which should be pretty rare). If we eventually change (as I am hoping) the storage of the ImmutableCFOptions to be ImmutableOptions, we will end up with some extra reference counting of the ImmutableDBOptions (one for each ColumnFamily instead of one for each DB). But this expense should generally be a one-time expense paid at DB (or ColumnFamily) creation and destruction and not a per-operation expense (most calls should either take in a ImmutableOptions pointer or reference, not a copy). |
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@mrambacher merged this pull request in 01e460d. |
This PR is a first step at attempting to clean up some of the Mutable/Immutable Options code. With this change, a DBOption and a ColumnFamilyOption can be reconstructed from their Mutable and Immutable equivalents, respectively.
readrandom tests do not show any performance degradation versus master (though both are slightly slower than the current 6.19 release).
There are still fields in the ImmutableCFOptions that are not CF options but DB options. Eventually, I would like to move those into an ImmutableOptions (= ImmutableDBOptions+ImmutableCFOptions). But that will be part of a future PR to minimize changes and disruptions.