Avoid updating DBOptions if there's no value updated #8518
Conversation
|
@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
options/db_options.cc
Outdated
| // check if new_options is changed by comparing to base_options | ||
| if (is_changed) { | ||
| std::string mismatch; | ||
| *is_changed = !OptionTypeInfo::StructsAreEqual( | ||
| config_options, "MutableDBOptions", &db_mutable_options_type_info, | ||
| "MutableDBOptions", &base_options, new_options, &mismatch); | ||
| } |
There was a problem hiding this comment.
I think this would be better as a new API (AreMutableDBOptionsEqual ?). This way, the DB code could do whatever sanitization is necessary before the comparison takes place. Effectively the same code, just in a new routine.
| // update again | ||
| ASSERT_OK(dbfull()->SetDBOptions({{"bytes_per_sync", "123"}})); | ||
| ASSERT_FALSE(is_changed()); | ||
|
|
There was a problem hiding this comment.
What about some tests that set bytes_per_sync=0?
There was a problem hiding this comment.
Added. With the checking after sanitization, it can avoid updating the value for setting it to 0. But the default value is not sanitized, but SetDBOptions() does. Not sure why, seems it's an inconsistency between DB open and SetDBOptions().
db/db_impl/db_impl.cc
Outdated
| &new_options, &is_changed); | ||
| if (!is_changed) { |
There was a problem hiding this comment.
See the note below about making this a new routine. Would allow for the sanitization in the following lines to take place before the comparison.
Summary: Try avoid expensive updating options operation if `SetDBOptions()` does not change any option value. Skip updating is not guaranteed, for example, changing `bytes_per_sync` to `0` may still trigger updating, as the value could be sanitized. Test Plan: added unittest
|
@jay-zhuang has updated the pull request. You must reimport the pull request before landing. |
|
@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
|
@jay-zhuang merged this pull request in 42eaa45. |
PR facebook#8518 merge the change to wrong section.
Summary: Try to avoid expensive updating DBOptions if
SetDBOptions()does not change any option value.Test Plan: added unittest