FindObsoleteFiles() to directly check whether candidate files are live #10040
Conversation
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks @siying for the PR.
Overall idea LGTM except for the test failures.
| @@ -242,6 +242,14 @@ void DBImpl::FindObsoleteFiles(JobContext* job_context, bool force, | |||
| log_file, immutable_db_options_.db_log_dir); | |||
| } | |||
| } | |||
| } else { | |||
| // Instead of filling ob_context->sst_live and job_context->blob_live, | |||
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
siying
changed the title
siying
force-pushed
the
speedup_find_obsolete_files
branch
from
310bd8f
to
52091e3
Compare
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@riversand963 I fixed a bug by changing the approach of dealing with obsolete files by adding a flag indicating whether the file needs to be deleted. Can you take another look? |
db/version_set.h
Outdated
| // be kept. This is because another FileMetaData still references the | ||
| // file, usually because the file is trivial moved so two FileMetadata | ||
| // is managing the file. | ||
| bool should_delete_file = true; |
There was a problem hiding this comment.
Nit: preferably initialize all members in the same manner.
db/db_impl/db_impl_files.cc
Outdated
| @@ -395,8 +403,10 @@ void DBImpl::PurgeObsoleteFiles(JobContext& state, bool schedule_only) { | |||
| state.manifest_delete_files.size()); | |||
| // We may ignore the dbname when generating the file names. | |||
| for (auto& file : state.sst_delete_files) { | |||
| candidate_files.emplace_back( | |||
| MakeTableFileName(file.metadata->fd.GetNumber()), file.path); | |||
| if (file.should_delete_file) { | |||
There was a problem hiding this comment.
I find the name here confusing: if a file is already in state_delete_files, why will should_delete_file be false? Only when I checked the definition of should_delete_file did I find out. I wonder whether we can have a better name than should_delete_file.
There was a problem hiding this comment.
File operations should not be on the critical path because it should happen infrequently if the db is configured properly, thus the overhead of std::shared_ptr should not be a concern here. I wonder why we didn't use smart pointers in the first place so that we did not have to worry about ref-counting in RocksDB codebase.
Clarification: this is a question/comment, not something we need to address in this PR.
There was a problem hiding this comment.
This is kind of an approach #8494 is taking, but I am a little bit worried about the side effect. The side effect would be that for now, with trivial move and max_open_files=-1, we will open the file twice with two table readers, and passing in different level information, and taking I/O latency histogram in the correct level. With the change of consolidating FileMetaData, some of the information will be wrong after trivial move. (this gets tricky as with max_open_files!=-1, they are sometimes wrong anyway). So it is a larger problem to solve, and it's too complicated to deal with together. If someone has time, perhaps we can have a design discussion to get this straight, perhaps with @yiwu-arbug too.
|
@siying has updated the pull request. You must reimport the pull request before landing. |
siying
force-pushed
the
speedup_find_obsolete_files
branch
from
c721e62
to
8d145c0
Compare
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…re live Summary: Right now, in FindObsoleteFiles() we build a list of all live SST files from all existing Versions. This is all done in DB mutex, and is O(m*n) where m is number of versions and n is number of files. In some extereme cases, it can take very long. The list is used to see whether a candidate file still shows up in a version. With this commit, every candidate file is directly check against all the versions for file existance. This operation would be O(m*k) where k is number of candidate files. Since is usually small (except perhaps full compaction in universal compaction), this is usually much faster than the previous solution. Test Plan: TBD
siying
force-pushed
the
speedup_find_obsolete_files
branch
from
8d145c0
to
c06641a
Compare
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
facebook#10040) Summary: Right now, in FindObsoleteFiles() we build a list of all live SST files from all existing Versions. This is all done in DB mutex, and is O(m*n) where m is number of versions and n is number of files. In some extereme cases, it can take very long. The list is used to see whether a candidate file still shows up in a version. With this commit, every candidate file is directly check against all the versions for file existance. This operation would be O(m*k) where k is number of candidate files. Since is usually small (except perhaps full compaction in universal compaction), this is usually much faster than the previous solution. Pull Request resolved: facebook#10040 Test Plan: TBD Reviewed By: riversand963 Differential Revision: D36613391 fbshipit-source-id: 3f13b090f755d9b3ae417faec62cd6e798bac1eb Signed-off-by: tabokie <xy.tao@outlook.com>
facebook#10040) Summary: Right now, in FindObsoleteFiles() we build a list of all live SST files from all existing Versions. This is all done in DB mutex, and is O(m*n) where m is number of versions and n is number of files. In some extereme cases, it can take very long. The list is used to see whether a candidate file still shows up in a version. With this commit, every candidate file is directly check against all the versions for file existance. This operation would be O(m*k) where k is number of candidate files. Since is usually small (except perhaps full compaction in universal compaction), this is usually much faster than the previous solution. Pull Request resolved: facebook#10040 Test Plan: TBD Reviewed By: riversand963 Differential Revision: D36613391 fbshipit-source-id: 3f13b090f755d9b3ae417faec62cd6e798bac1eb Signed-off-by: tabokie <xy.tao@outlook.com>
| path = std::move(rhs.path); | ||
| metadata = rhs.metadata; | ||
| rhs.metadata = nullptr; | ||
|
|
There was a problem hiding this comment.
Should we have only_delete_metadata = rhs.only_delete_metadata here too? For that, a potential merging conflict with #9924.
There was a problem hiding this comment.
Indeed. It didn't cause any alarm because JobContext::sst_delete_files isn't moved or resized after its only_delete_metadata is set.
facebook#10040) Summary: Right now, in FindObsoleteFiles() we build a list of all live SST files from all existing Versions. This is all done in DB mutex, and is O(m*n) where m is number of versions and n is number of files. In some extereme cases, it can take very long. The list is used to see whether a candidate file still shows up in a version. With this commit, every candidate file is directly check against all the versions for file existance. This operation would be O(m*k) where k is number of candidate files. Since is usually small (except perhaps full compaction in universal compaction), this is usually much faster than the previous solution. Pull Request resolved: facebook#10040 Test Plan: TBD Reviewed By: riversand963 Differential Revision: D36613391 fbshipit-source-id: 3f13b090f755d9b3ae417faec62cd6e798bac1eb Signed-off-by: tabokie <xy.tao@outlook.com>
facebook#10040) Summary: Right now, in FindObsoleteFiles() we build a list of all live SST files from all existing Versions. This is all done in DB mutex, and is O(m*n) where m is number of versions and n is number of files. In some extereme cases, it can take very long. The list is used to see whether a candidate file still shows up in a version. With this commit, every candidate file is directly check against all the versions for file existance. This operation would be O(m*k) where k is number of candidate files. Since is usually small (except perhaps full compaction in universal compaction), this is usually much faster than the previous solution. Pull Request resolved: facebook#10040 Test Plan: TBD Reviewed By: riversand963 Differential Revision: D36613391 fbshipit-source-id: 3f13b090f755d9b3ae417faec62cd6e798bac1eb Signed-off-by: tabokie <xy.tao@outlook.com>
Summary: Right now, in FindObsoleteFiles() we build a list of all live SST files from all existing Versions. This is all done in DB mutex, and is O(mn) where m is number of versions and n is number of files. In some extereme cases, it can take very long. The list is used to see whether a candidate file still shows up in a version. With this commit, every candidate file is directly check against all the versions for file existance. This operation would be O(mk) where k is number of candidate files. Since is usually small (except perhaps full compaction in universal compaction), this is usually much faster than the previous solution.
Test Plan: TBD