Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit sst in dedicated threads #1275

Open
wants to merge 2 commits into
base: fb-mysql-8.0.28
Choose a base branch
from

Conversation

rockeet
Copy link
Contributor

@rockeet rockeet commented Mar 1, 2023

Execute Rdb_sst_info::commit_sst_file in dedicated threads, this improves performance:

  1. Rdb_sst_file_ordered::commit may use stack to reverse input data, this is time consuming
  2. m_sst_file_writer->Finish may be consuming, at least it need to call fsync

Execute Rdb_sst_info::commit_sst_file in dedicated threads increase parallelization with mimimal code changes.

@@ -392,13 +393,23 @@ int Rdb_sst_info::open_new_sst_file() {
}

void Rdb_sst_info::commit_sst_file(Rdb_sst_file_ordered *sst_file) {
m_commiting_threads_mutex.lock();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider std::lock_guardstd::mutex? we use RDB_MUTEX_LOCK_CHECK macros elsewhere in this file

@@ -479,6 +490,11 @@ int Rdb_sst_info::finish(Rdb_sst_commit_info *commit_info,
close_curr_sst_file();
}

for (auto& thr : m_commiting_threads) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to grab a log here too?

@bladepan
Copy link
Contributor

Execute Rdb_sst_info::commit_sst_file in dedicated threads, this improves performance:

1. `Rdb_sst_file_ordered::commit` may use stack to reverse input data, this is time consuming

2. `m_sst_file_writer->Finish` may be consuming, at least it need to call `fsync`

Execute Rdb_sst_info::commit_sst_file in dedicated threads increase parallelization with mimimal code changes.

  1. the keys are already ordered, why it would reverse input data?
  2. the fsync happens when writing sst file to disk?

could you please provide some rough calculations about the time savings?

@rockeet
Copy link
Contributor Author

rockeet commented Apr 26, 2023

Execute Rdb_sst_info::commit_sst_file in dedicated threads, this improves performance:

1. `Rdb_sst_file_ordered::commit` may use stack to reverse input data, this is time consuming

2. `m_sst_file_writer->Finish` may be consuming, at least it need to call `fsync`

Execute Rdb_sst_info::commit_sst_file in dedicated threads increase parallelization with mimimal code changes.

  1. the keys are already ordered, why it would reverse input data?
  2. the fsync happens when writing sst file to disk?

could you please provide some rough calculations about the time savings?

  1. In Rdb_sst_file_ordered::commit(), if m_use_stack is true, it will write kv from stack.
  2. In Rdb_sst_file_ordered::Rdb_sst_file::commit(), it calls m_sst_file_writer->Finish(), in which will write file and call fsync.

In our private branch, when m_use_stack is true(reverse cf), create index time reduces 30+%.

@bladepan
Copy link
Contributor

1. In `Rdb_sst_file_ordered::commit()`, if `m_use_stack` is true, it will write kv from stack.

2. In `Rdb_sst_file_ordered::Rdb_sst_file::commit()`, it calls `m_sst_file_writer->Finish()`, in which will write file and call fsync.

In our private branch, when m_use_stack is true(reverse cf), create index time reduces 30+%.

Rdb_index_merge and Rdb_sst_file_ordered both use cf's comparator, when Rdb_index_merge write to Rdb_sst_file_ordered, the keys should be in cf's increasing order, m_use_stack should be false in this case.

@rockeet
Copy link
Contributor Author

rockeet commented Apr 28, 2023

1. In `Rdb_sst_file_ordered::commit()`, if `m_use_stack` is true, it will write kv from stack.

2. In `Rdb_sst_file_ordered::Rdb_sst_file::commit()`, it calls `m_sst_file_writer->Finish()`, in which will write file and call fsync.

In our private branch, when m_use_stack is true(reverse cf), create index time reduces 30+%.

Rdb_index_merge and Rdb_sst_file_ordered both use cf's comparator, when Rdb_index_merge write to Rdb_sst_file_ordered, the keys should be in cf's increasing order, m_use_stack should be false in this case.

We removed merge_record::m_comparator since it is identical in each std::set element, and using Slice::operator< to avoid rocksdb::Comparator virtual function call, thus it is reversed with regard to rev cf and makes m_use_stack being true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants