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

Notify caller when apply has finished #4424

Closed

Conversation

zhangjinpeng87
Copy link
Member

@zhangjinpeng87 zhangjinpeng87 commented Mar 22, 2019

Signed-off-by: zhangjinpeng1987 zhangjinpeng@pingcap.com

What have you changed? (mandatory)

Previous behavior for prewrite/commit request:

  1. scheduler receive a request and acquire latches.
  2. scheduler send request to local reader ask for a snapshot.
  3. local reader fetch a snapshot and redirect request to scheduler-worker-pool.
  4. scheduler-worker-pool check txn constraints and generate modifications and send to raftstore.
  5. after raftstore(apply worker) has applied the modifications, raftstore send a result back to scheduler.
  6. scheduler release latches and notify the caller.

This branch change step 5, raftstore notify the caller and send a message to scheduler after modifications has been applied. So the latency for prewrite/commit can be reduced.

What are the type of the changes? (mandatory)

The currently defined types are listed below, please pick one of the types for this PR by removing the others:

  • Improvement (change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Unit tests

Does this PR affect documentation (docs) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

  • Benchmark
insert threads master (QPS/avg latency/.99 latency) branch
1 526 / 1.89 / 2.43 541 / 1.84 / 2.35
2 1211 / 1.65 / 2.18 1239 / 1.61 / 2.52
4 2655 / 1.5 / 2.22 2651 / 1.51 / 2.11
16 7792 / 2.05 / 4.33 7844 / 2.04 / 4.33
32 10482 / 3.05 / 8.74 10499 / 3.04 / 8.38

apply-notify-cpu
As showed above:

  • When the insert load is low, this feature can reduce latency by about 2%.
  • Because the callbacks are involved by the apply pool, so the scheduler CPU reduced by 10%, and this part of cost transferred to apply pool, i think this is helpful when the single thread scheduler is the bottleneck.

Signed-off-by: zhangjinpeng1987 <zhangjinpeng@pingcap.com>
cb_ctx: CbContext,
snapshot: EngineResult<E::Snap>,
task: Task,
storage_cb: Arc<Mutex<Option<StorageCb>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

em, seem we can use crossbeam AtomicOption

Copy link
Member Author

Choose a reason for hiding this comment

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

Good, i will have a try.

Copy link
Member Author

Choose a reason for hiding this comment

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

crossbeam 0.5 abandoned AtomicOption.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, bad news, AtomicOptino looks better...
@brson do you have any other suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could use AtomicCell<Option<T>> in place of AtomicOption<T> (see crossbeam-rs/crossbeam#186)

Copy link
Contributor

Choose a reason for hiding this comment

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

AtomicCell looks like the right choice. I'd also assert that AtomicCell::is_lock_free, to make sure that Option<StorageCb> stays under a word long.

@breezewish breezewish added the component/storage Component: Storage, Scheduler, etc. label Mar 26, 2019
@siddontang
Copy link
Contributor

i think this is helpful when the single thread scheduler is the bottleneck.

I think we will remove the single thread scheduler later, maybe you can let @hicqu benchmark in his PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/storage Component: Storage, Scheduler, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants