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 seek_write in prewrite #5846

Merged
merged 15 commits into from Nov 27, 2019
Merged

Conversation

@Little-Wallace
Copy link
Contributor

Little-Wallace commented Nov 8, 2019

What have you changed?

I'm try to optimize check conflict of prewrite by ScanMode::Forward. If there are many keys to be inserted, they must not exist in CF_WRITE, so we could seek once , and skip seeking others.

What is the type of the changes?

Pick one of the following and delete the others:

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

How is the PR tested?

  • Unit test

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No

Does this PR affect tidb-ansible?

No

src/storage/mvcc/txn.rs Outdated Show resolved Hide resolved
src/storage/txn/process.rs Outdated Show resolved Hide resolved
@tabokie

This comment has been minimized.

Copy link
Member

tabokie commented Nov 11, 2019

/test

@Little-Wallace Little-Wallace force-pushed the Little-Wallace:prewrite_seek branch from a292fbf to 4368567 Nov 11, 2019
@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 11, 2019

/test

@Little-Wallace Little-Wallace removed the S: WIP label Nov 11, 2019
@Little-Wallace Little-Wallace requested a review from youjiali1995 Nov 11, 2019
@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 11, 2019

/test

@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 12, 2019

@@ -113,6 +113,7 @@ impl<I: Iterator> Cursor<I> {
{
return Ok(true);
}

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 12, 2019

Contributor

blank line?

// ScanMode is `None`, since in prewrite and other operations, keys are not given in
// order and we use prefix seek for each key. An exception is GC, which uses forward
// scan only.
Comment on lines 48 to 47

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 12, 2019

Contributor

Need modification.


let mut txn =
MvccTxn::new(snapshot.clone(), None, start_ts, !ctx.get_not_fill_cache())?;
let use_forward = rows > FORWARD_MIN_MUTATIONS_NUM;

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 12, 2019

Contributor

Prefer maybe_use_forward

let mut mutations = Vec::default();
let write_num: usize = FORWARD_MIN_MUTATIONS_NUM + 2;
let pri_key_number = 2;
let pri_key = pri_key_number.to_string().as_bytes().to_vec();

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 12, 2019

Contributor

pri_key_number.to_string().into_bytes()

@@ -2031,6 +2055,95 @@ mod tests {
);
}

#[test]
fn test_txn_prewrite_insert() {

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 12, 2019

Contributor

I think you can add a test in src/storage/txn/process.rs which checks the statistics returned by Executor::process_write().

This comment has been minimized.

Copy link
@Little-Wallace

Little-Wallace Nov 12, 2019

Author Contributor

But all other case about transaction are tested in src/storage/mod.rs.

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 15, 2019

Contributor

If the test is for some details in process.rs, I think it's not a bad idea to put it in process.rs.

@youjiali1995

This comment has been minimized.

Copy link
Contributor

youjiali1995 commented Nov 12, 2019

/bench

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 12, 2019

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: 8404a430c11c6076b6f7e757c420e8a362ca2d08
--- tikv: ce273530df2ecdf3c4092edef29dae57f2341e6c
+++ tikv: 41bb21cadc684b8fb9d63b7fa8e0c987b9d610b3
pd: 3d3832d046ecc2f35077e69b6e604bc226e6f155
================================================================================
oltp_update_non_index:
    * QPS: 4712.98 ± 0.20% (std=6.87) delta: -0.02% (p=0.864)
    * Latency p50: 27.15 ± 0.20% (std=0.04) delta: 0.02%
    * Latency p99: 42.43 ± 4.09% (std=1.14) delta: -1.81%
            
oltp_insert:
    * QPS: 4782.76 ± 0.09% (std=2.89) delta: -0.15% (p=0.299)
    * Latency p50: 26.76 ± 0.08% (std=0.01) delta: 0.14%
    * Latency p99: 48.63 ± 1.19% (std=0.41) delta: 7.95%
            
oltp_read_write:
    * QPS: 15419.00 ± 0.04% (std=4.82) delta: -1.04% (p=0.008)
    * Latency p50: 166.33 ± 0.05% (std=0.06) delta: 1.03%
    * Latency p99: 312.65 ± 2.98% (std=6.99) delta: 0.01%
            
oltp_update_index:
    * QPS: 4234.53 ± 0.10% (std=3.19) delta: 0.41% (p=0.781)
    * Latency p50: 30.22 ± 0.12% (std=0.03) delta: -0.43%
    * Latency p99: 54.84 ± 3.64% (std=1.22) delta: -0.43%
            
oltp_point_select:
    * QPS: 45153.20 ± 0.27% (std=86.00) delta: -0.19% (p=0.687)
    * Latency p50: 2.83 ± 0.18% (std=0.00) delta: 0.29%
    * Latency p99: 9.14 ± 0.88% (std=0.08) delta: 1.48%
            
@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 14, 2019

@youjiali1995 @MyonKeminta Any more comments?

@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 14, 2019

/bench

@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 14, 2019

/test

1 similar comment
@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 14, 2019

/test

@youjiali1995

This comment has been minimized.

Copy link
Contributor

youjiali1995 commented Nov 14, 2019

Sorry. I and @MyonKeminta are a little busy.

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 14, 2019

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: 2aa571b1abf329d336413fe195c88fe07c7b268c
--- tikv: 016d0cb71707e413ea3a2d9bba5980e2ce74fc42
+++ tikv: 52af301667eb83a73fc00f834f69b40d1cd5afd3
pd: 5b831f622e8a4073e910fc1a90d359b1e12f5f46
================================================================================
oltp_update_non_index:
    * QPS: 4707.65 ± 0.28% (std=9.22) delta: -0.06% (p=0.467)
    * Latency p50: 27.19 ± 0.29% (std=0.05) delta: 0.06%
    * Latency p99: 41.60 ± 1.20% (std=0.35) delta: -3.56%
            
oltp_insert:
    * QPS: 4766.24 ± 0.10% (std=3.25) delta: -0.18% (p=0.015)
    * Latency p50: 26.85 ± 0.10% (std=0.02) delta: 0.19%
    * Latency p99: 49.23 ± 1.80% (std=0.88) delta: 3.63%
            
oltp_read_write:
    * QPS: 15439.92 ± 0.62% (std=63.59) delta: 0.05% (p=0.440)
    * Latency p50: 166.18 ± 0.59% (std=0.69) delta: -0.01%
    * Latency p99: 311.70 ± 2.72% (std=6.28) delta: -0.02%
            
oltp_update_index:
    * QPS: 4224.32 ± 0.07% (std=2.20) delta: 0.04% (p=0.374)
    * Latency p50: 30.30 ± 0.08% (std=0.02) delta: -0.04%
    * Latency p99: 53.86 ± 3.63% (std=1.20) delta: 1.84%
            
oltp_point_select:
    * QPS: 45633.90 ± 0.09% (std=29.69) delta: 0.57% (p=0.188)
    * Latency p50: 2.80 ± 0.00% (std=0.00) delta: -0.62%
    * Latency p99: 9.06 ± 0.00% (std=0.00) delta: 0.00%
            
@tabokie

This comment has been minimized.

Copy link
Member

tabokie commented Nov 14, 2019

/bench

workload:
  threads: 1024
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 14, 2019

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: ec95f91c139107762a7eadc1d23a1fb0a8ccee2d
--- tikv: 4ff5c85bc3574b0d64d68d8b2a4536be9a856258
+++ tikv: 4df89199390b883c185834545008002d21d6f1f9
pd: 5b831f622e8a4073e910fc1a90d359b1e12f5f46
================================================================================
oltp_update_non_index:
    * QPS: 15454.78 ± 0.55% (std=65.70) delta: 0.54% (p=0.114)
    * Latency p50: 66.24 ± 0.55% (std=0.28) delta: -0.52%
    * Latency p99: 149.40 ± 1.20% (std=1.26) delta: -0.59%
            
oltp_insert:
    * QPS: 12023.07 ± 0.33% (std=28.08) delta: 0.17% (p=0.227)
    * Latency p50: 85.14 ± 0.32% (std=0.20) delta: -0.17%
    * Latency p99: 143.05 ± 2.23% (std=2.12) delta: 0.46%
            
oltp_read_write:
    * QPS: 15977.36 ± 0.04% (std=4.67) delta: 0.29% (p=0.117)
    * Latency p50: 1301.51 ± 0.11% (std=0.97) delta: -0.45%
    * Latency p99: 2493.86 ± 0.00% (std=0.00) delta: -3.11%
            
oltp_update_index:
    * QPS: 9065.65 ± 0.54% (std=29.89) delta: -0.41% (p=0.350)
    * Latency p50: 111.62 ± 0.69% (std=0.54) delta: 0.38%
    * Latency p99: 228.45 ± 2.24% (std=3.40) delta: 0.46%
            
oltp_point_select:
    * QPS: 49552.26 ± 0.43% (std=193.52) delta: -0.06% (p=0.815)
    * Latency p50: 20.65 ± 0.44% (std=0.08) delta: 0.05%
    * Latency p99: 52.89 ± 0.00% (std=0.00) delta: 0.00%
            
@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Nov 14, 2019

how about sysbench prepare?

@Little-Wallace

This comment has been minimized.

Copy link
Contributor Author

Little-Wallace commented Nov 14, 2019

@siddontang It makes no difference to the performance of sysbench prepare with default config (because it costs too much time to wait for latch), but reduce 25% CPU (from 880% to 630%).

@@ -140,7 +140,7 @@ impl<S: Snapshot> MvccReader<S> {
}

let cursor = self.write_cursor.as_mut().unwrap();
let ok = cursor.near_seek(&key.clone().append_ts(ts), &mut self.statistics.write)?;
let ok = cursor.seek(&key.clone().append_ts(ts), &mut self.statistics.write)?;

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Nov 15, 2019

Member

Why not near_seek?

This comment has been minimized.

Copy link
@Little-Wallace

Little-Wallace Nov 15, 2019

Author Contributor

Because the keys which we are ready to prewrite are not adjacent, we may call too many times next in near_seek.
I will add a new interface to use seek instead of near_seek.

@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Nov 15, 2019

but reduce 25% CPU (from 880% to 630%).

it is great too @Little-Wallace

for x in res.iter() {
match x {
Ok(_) => (),
_e @ Err(Error::Txn(txn::Error::Mvcc(MvccError::KeyIsLocked { .. }))) => {

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 15, 2019

Contributor

If you don't need the error value, just remove the _e @

Suggested change
_e @ Err(Error::Txn(txn::Error::Mvcc(MvccError::KeyIsLocked { .. }))) => {
Err(Error::Txn(txn::Error::Mvcc(MvccError::KeyIsLocked { .. }))) => {
@@ -2031,6 +2055,95 @@ mod tests {
);
}

#[test]
fn test_txn_prewrite_insert() {

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 15, 2019

Contributor

If the test is for some details in process.rs, I think it's not a bad idea to put it in process.rs.

let pri_key = pri_key_number.to_string().as_bytes().to_vec();
for i in 0..write_num {
mutations.push(Mutation::Insert((
Key::from_raw(i.to_string().as_bytes()),

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 15, 2019

Contributor

Although it doesn't matter in this PR, you need to be aware that "10" < "2", if write_num > 10. So I prefer use &[i] as the key rather than i.to_string().

let mut mutations = Vec::default();
for i in 0..write_num {
if i == pri_key_number {
mutations.push(Mutation::Put((
Key::from_raw(i.to_string().as_bytes()),
b"100".to_vec(),
)));
} else {
mutations.push(Mutation::Insert((
Key::from_raw(i.to_string().as_bytes()),
b"100".to_vec(),
)));
}
}
Comment on lines 2132 to 2145

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 15, 2019

Contributor

You can just mutate mutations[pri_key_number] and don't need to create the whole thing again.

pub fn seek_write(&mut self, key: &Key, ts: u64) -> Result<Option<(u64, Write)>> {
return self.inner_seek_write(key, ts, false);
}

Comment on lines 162 to 165

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 15, 2019

Contributor

Did you check all usages of the old seek_write function?

This comment has been minimized.

Copy link
@Little-Wallace

Little-Wallace Nov 16, 2019

Author Contributor

Yes。 Because I use Goland doing this replace operation.

let rows = mutations.len();

let scan_mode = if options.for_update_ts == 0 && rows > FORWARD_MIN_MUTATIONS_NUM {

This comment has been minimized.

Copy link
@MyonKeminta

MyonKeminta Nov 15, 2019

Contributor

Please add a comment to explain the condition.

src/storage/txn/process.rs Outdated Show resolved Hide resolved
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
fix fmt
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
let iter_opt = IterOption::new(None, None, true);
let mut iter = snapshot.iter_cf(cf, iter_opt, ScanMode::Forward)?;
if iter.seek(left, statistic)? {
if iter.key(statistic) < right.as_encoded().as_slice() {

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Nov 26, 2019

Member

Better to give explanation for the function, the check range is [left, right).

let rows = mutations.len();
if options.for_update_ts.is_zero() && rows > FORWARD_MIN_MUTATIONS_NUM {
mutations.sort_by(|a, b| a.key().cmp(b.key()));
let left_key = mutations.first().unwrap().key().clone().append_ts(start_ts);

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Nov 26, 2019

Member

Don't need to append ts.

youjiali1995 and others added 3 commits Nov 26, 2019
Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Copy link
Member

zhangjinpeng1987 left a comment

LGTM

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Ok(Self {
// Use `ScanMode::Forward` when gc or prewrite with multiple `Mutation::Insert`,
// which would seek less times.
// When `scan_mode` is `Some(ScanMode::Forward)`, all keys must be writte by

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 26, 2019

Contributor
Suggested change
// When `scan_mode` is `Some(ScanMode::Forward)`, all keys must be writte by
// When `scan_mode` is `Some(ScanMode::Forward)`, all keys must be passed
let rows = mutations.len();
if options.for_update_ts.is_zero() && rows > FORWARD_MIN_MUTATIONS_NUM {
mutations.sort_by(|a, b| a.key().cmp(b.key()));
let left_key = mutations.first().unwrap().key().clone();

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 26, 2019

Contributor

Needn't call clone().


#[test]
fn test_process_write_impl_prewrite() {
inner_test_process_write_impl_conflict(0, FORWARD_MIN_MUTATIONS_NUM + 1);

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 26, 2019

Contributor

What's the difference between these tests?

This comment has been minimized.

Copy link
@Little-Wallace

Little-Wallace Nov 26, 2019

Author Contributor

Check conflict key which is On the boundary of the interval.

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 26, 2019

Contributor

Oh, I see.

}

#[test]
fn test_process_write_impl_prewrite() {

This comment has been minimized.

Copy link
@youjiali1995

youjiali1995 Nov 26, 2019

Contributor

I think test_prewrite_skip_constraint_check is more proper.

Signed-off-by: Little-Wallace <bupt2013211450@gmail.com>
Copy link
Contributor

youjiali1995 left a comment

LGTM

youjiali1995 and others added 2 commits Nov 26, 2019
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 26, 2019

/run-all-tests

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Nov 26, 2019

@Little-Wallace merge failed.

@youjiali1995

This comment has been minimized.

Copy link
Contributor

youjiali1995 commented Nov 26, 2019

/test

Copy link
Member

zhangjinpeng1987 left a comment

LGTM

@zhangjinpeng1987 zhangjinpeng1987 merged commit 1ccd814 into tikv:master Nov 27, 2019
3 checks passed
3 checks passed
DCO All commits are signed off!
Details
idc-jenkins-ci-tikv/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
hawkingrei added a commit to hawkingrei/tikv that referenced this pull request Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.