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

executor: do not read from lock cache when snapshot read (#21529) #21539

Merged
merged 3 commits into from
Dec 8, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #21529 to release-4.0


Signed-off-by: you06 you1474600@gmail.com

What problem does this PR solve?

Issue Number: close #21447

Problem Summary:

What is changed and how it works?

What's Changed:

When read from snapshot and key not exist in membuffer, do not look up from lock cache.

How it Works:

Lock key will write lock cache, if the key is locked but the value is not affected the locked value will only be written into lock cache, the cached value should not read by a snapshot read.

Related changes

  • Need to cherry-pick to the release-4.0

Check List

Tests

  • Unit test

Release note

  • No release note

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@you06 you're already a collaborator in bot's repo.

Signed-off-by: you06 <you1474600@gmail.com>
@you06
Copy link
Contributor

you06 commented Dec 7, 2020

/run-all-tests

@cfzjywxk
Copy link
Contributor

cfzjywxk commented Dec 7, 2020

Seems batch point get processing is missing?

Signed-off-by: you06 <you1474600@gmail.com>
@you06
Copy link
Contributor

you06 commented Dec 7, 2020

/run-all-tests

@you06
Copy link
Contributor

you06 commented Dec 7, 2020

Seems batch point get processing is missing?

Fixed.

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 8, 2020
@cfzjywxk
Copy link
Contributor

cfzjywxk commented Dec 8, 2020

@jebter I think we need to do some benchmark after merging this PR, maybe there will be some benchmark improvement relying on the mistakenly used pessimistic lock cache, for example like

select * from t where pk = 1 for update ;
select * from t where pk = 1;
select * from t where pk = 1;

There may be some regression for this kind of benchmark.

@cfzjywxk
Copy link
Contributor

cfzjywxk commented Dec 8, 2020

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 8, 2020
@you06
Copy link
Contributor

you06 commented Dec 8, 2020

Here's the result testing with https://gist.github.com/you06/ac94678ccfb69448238e31955ebd5350

thread count 689914f this PR
1 1024 0.526ms/1896 1.274ms/784
2 1024 0.402ms/4958 1.087ms/1837
4 1024 0.350ms/11399 1.134ms/3521
8 1024 0.353ms/22569 1.008ms/7924
16 1024 0.469ms/34003 1.010ms/15824
32 1024 0.662ms/48259 1.456ms/21955
64 1024 1.073ms/59572 2.079ms/30766
128 1024 1.991ms/64261 3.348ms/38216
256 1024 3.132ms/81699 5.188ms/49333
512 1024 6.483ms/78963 9.363ms/54674

The latency reading from lock cache is much lower, however point get from tikv seems efficient enough.

@cfzjywxk cc

@cfzjywxk cfzjywxk modified the milestones: 4.0.0, v4.0.9 Dec 8, 2020
@jackysp jackysp merged commit b24975d into pingcap:release-4.0 Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/transaction SIG:Transaction status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants