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

Optimize Commit pipeline performance #286

Merged
merged 8 commits into from
Jun 21, 2022
Merged

Conversation

ethercflow
Copy link
Member

@ethercflow ethercflow commented May 20, 2022

Follow up on #267

Optimize the waiting delay caused by the uneven load of the writer wb: the writer who enters the request queue later enters the commit phase first.

Signed-off-by: Wenbo Zhang ethercflow@gmail.com

@ethercflow
Copy link
Member Author

@Connor1996 @tabokie PTAL, thanks!

@tabokie
Copy link
Member

tabokie commented May 20, 2022

Please add some descriptions for the new algorithm. I didn't find any on the weekly report.

@ethercflow
Copy link
Member Author

Please add some descriptions for the new algorithm. I didn't find any on the weekly report.

  1. Create a task queue for each writer, the difference with MultiWriteBatch is that the latter shares the queue;
  2. After each writer confirms the order of submission in the request queue, it loops through its own task queue. The definition of each task is the same as that of MultiWriteBatch;
  3. If the head of the request queue completes the memtable write operation first and enters the commit stage, it can return directly without waiting for other writers. This behavior is consistent with the previous pipelined commit;
  4. If the non-head writers first enter the commit phase and find that the task queue of the head writer has pending tasks, the loop will take out tasks from the task queue of the head writer for execution;
    4.1 After the header is completed, it is found that the helpers are also completed, then pop these writers from the request queue in turn, and notify the helpers to return;
    4.2 If the header completes, but finds that the tasks the helpers performed for him have not yet been processed, wait for the helpers to complete and wake it up to clean up from the request queue.

Sorry I missed the email, I've add the descriptions. Please let me know if it's clear enough, thanks! @tabokie @Connor1996

@Connor1996 Connor1996 self-requested a review June 1, 2022 07:18
util/safe_queue.h Outdated Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
db/db_impl/db_impl.h Outdated Show resolved Hide resolved
db/write_thread.cc Outdated Show resolved Hide resolved
db/write_thread.cc Outdated Show resolved Hide resolved
@Connor1996

This comment was marked as resolved.

@ethercflow

This comment was marked as resolved.

util/safe_queue.h Outdated Show resolved Hide resolved
util/safe_queue.h Outdated Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
@ethercflow
Copy link
Member Author

ethercflow commented Jun 14, 2022

Hi @tabokie @Connor1996 I've updated a version according to your suggestions (except for the supplementary comments, which will be added later), PTAL again, thank you.

table/table_test.cc Show resolved Hide resolved
db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
db/write_thread.cc Outdated Show resolved Hide resolved
Copy link
Member Author

@ethercflow ethercflow left a comment

Choose a reason for hiding this comment

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

I have a few questions about how to deal with memtable write error:

  1. What will happen if write WAL successfully, but write memtable failed?
  2. We loop write memtable, according to the current logic, only the latest error is recorded, it this expected?
  3. The current helper thread does not call WriteStatusCheck(writer.status); after helping to execute wb, is this ok? (This function is a protected member defined in class db_impl, which is more troublesome to call in write_thread.cc)

@tabokie
Copy link
Member

tabokie commented Jun 15, 2022

  1. Do nothing and return the error. RocksDB will ignore the error when replaying logs during recovery (if paranoid_checks is not set).
  2. I don't think it's a big issue. Memtable don't err unless user writes are seriously malformed.
  3. Could you run the check after commit? at which point all tasks should be completed.

@ethercflow
Copy link
Member Author

  1. Do nothing and return the error. RocksDB will ignore the error when replaying logs during recovery (if paranoid_checks is not set).
  2. I don't think it's a big issue. Memtable don't err unless user writes are seriously malformed.

Got it, thanks!

  1. Could you run the check after commit? at which point all tasks should be completed.

I think it's OK.

@ethercflow ethercflow requested a review from tabokie June 15, 2022 10:46
Copy link
Member

@tabokie tabokie left a comment

Choose a reason for hiding this comment

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

I suggest adding another atomic flag persisted to mark that a request is safe to be applied. It can solve the two issues in my comments.

db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/version_set.cc Outdated Show resolved Hide resolved
db/write_thread.cc Show resolved Hide resolved
db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/db_impl/db_impl_write.cc Outdated Show resolved Hide resolved
db/write_batch_internal.h Outdated Show resolved Hide resolved
db/write_thread.cc Outdated Show resolved Hide resolved
db/write_thread.cc Outdated Show resolved Hide resolved
db/write_thread.cc Outdated Show resolved Hide resolved
db/write_thread.h Outdated Show resolved Hide resolved
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

rest LGTM

@ethercflow ethercflow requested a review from tabokie June 20, 2022 08:06
@tabokie
Copy link
Member

tabokie commented Jun 20, 2022

Might need to cherry-pick to 6.29.tikv branch.

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
ethercflow and others added 7 commits June 20, 2022 06:34
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
  - Move atomic ops into functions
  - Unified replacement of PebbleWrite with MultiBatchWrite
  - Add a few comments

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
@ethercflow
Copy link
Member Author

/run-test

@tabokie tabokie merged commit e4bfc11 into tikv:6.4.tikv Jun 21, 2022
tabokie added a commit to tabokie/rocksdb that referenced this pull request Jun 27, 2022
* Optimize Commit pipeline performance

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Remove safe queue, iterate the wbs directly

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Refactored some code
  - Move atomic ops into functions
  - Unified replacement of PebbleWrite with MultiBatchWrite
  - Add a few comments

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Reset pending_wb_cnt before wakeup writers when write WAL failed

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Update db/db_impl/db_impl_write.cc

Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Update db/write_thread.cc

Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Refactor some code

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Fix two bugs about write memtable

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie added a commit that referenced this pull request Jun 28, 2022
* Optimize Commit pipeline performance

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Remove safe queue, iterate the wbs directly

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Refactored some code
  - Move atomic ops into functions
  - Unified replacement of PebbleWrite with MultiBatchWrite
  - Add a few comments

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Reset pending_wb_cnt before wakeup writers when write WAL failed

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Update db/db_impl/db_impl_write.cc

Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Update db/write_thread.cc

Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Refactor some code

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Fix two bugs about write memtable

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

Co-authored-by: Xinye Tao <xy.tao@outlook.com>
Signed-off-by: tabokie <xy.tao@outlook.com>
ethercflow added a commit to ethercflow/rocksdb that referenced this pull request Aug 10, 2022
This reverts commit e4bfc11.

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
tabokie pushed a commit that referenced this pull request Aug 11, 2022
* Revert "Fix SIGABRT caused by uninitialized mutex (#296)"

This reverts commit 4037bda.

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

* Revert "Optimize Commit pipeline performance (#286)"

This reverts commit e4bfc11.

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>

Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants