Navigation Menu

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

*: use the revertable sandbox to buffer mutations #15931

Merged
merged 8 commits into from Apr 3, 2020

Conversation

bobotu
Copy link
Contributor

@bobotu bobotu commented Mar 31, 2020

What problem does this PR solve?

After #15780, this PR update the other parts of TiDB to use the new Flush and Discard API.

What is changed and how it works?

Update the MemBuffer interface:

  • Add Flush and Discard respect to Sandbox's API
  • Add NewBuffer to derive a new Sandbox
  • Remove Reset and SetCap because there is no need to use them

The TxnState's buffer now directly derived from the underlying Transaction when its state changed to valid. When statement commit/rollback the buffer will be discarded and a new Sandbox will be created (this allocation can be reduced by pooling temporal Sandbox, I'll submit a separate PR to add this optimization)

@bobotu bobotu requested a review from a team as a code owner March 31, 2020 14:47
@ghost ghost requested review from qw4990 and removed request for a team March 31, 2020 14:47
@bobotu bobotu added the sig/transaction SIG:Transaction label Mar 31, 2020
@bobotu bobotu requested review from tiancaiamao, coocood and jackysp and removed request for qw4990 March 31, 2020 14:48
@bobotu
Copy link
Contributor Author

bobotu commented Mar 31, 2020

/run-all-tests

@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Mar 31, 2020
@bobotu
Copy link
Contributor Author

bobotu commented Mar 31, 2020

/run-all-tests

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #15931 into master will increase coverage by 0.4476%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #15931        +/-   ##
================================================
+ Coverage   80.4524%   80.9001%   +0.4476%     
================================================
  Files           506        506                
  Lines        135393     137577      +2184     
================================================
+ Hits         108927     111300      +2373     
+ Misses        17958      17784       -174     
+ Partials       8508       8493        -15

@bobotu
Copy link
Contributor Author

bobotu commented Apr 1, 2020

/run-all-tests

3 similar comments
@bobotu
Copy link
Contributor Author

bobotu commented Apr 1, 2020

/run-all-tests

@bobotu
Copy link
Contributor Author

bobotu commented Apr 1, 2020

/run-all-tests

@bobotu
Copy link
Contributor Author

bobotu commented Apr 1, 2020

/run-all-tests

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

It's confusing that we see MemBuffer everywhere and we don't know which level of the MemBuffer is.
It would be more clear if we rename the MemBuffer type fields to stmtBuffer, txnBuffer, rowBuffer.

kv/kv.go Outdated
//
// Note: you cannot modify this MemBuffer until the child buffer finished,
// otherwise the Set operation will panic.
NewBuffer() MemBuffer
Copy link
Member

Choose a reason for hiding this comment

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

NewChildBuffer may be more clear.

@bobotu
Copy link
Contributor Author

bobotu commented Apr 2, 2020

/run-all-tests

@bobotu
Copy link
Contributor Author

bobotu commented Apr 2, 2020

/bench +tpcc +sysbench

@sre-bot
Copy link
Contributor

sre-bot commented Apr 2, 2020

Benchmark Report

Run TPC-C Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 827edde58535ac585b4b01473a854f1d923bb54a
+++ tidb: f7fcdc25da9292b539904b4998960761faa49b57
tikv: 50397a6e9f9e4f80ac640ec2b4bf6b1315ebb9d7
pd: fccb3d434acccb7256cacdd7c958a5996c2d870d
================================================================================
Measured tpmC (NewOrders): 7158.96 ± 1.05% (std=47.40), delta: 0.02% (p=0.978)

@sre-bot
Copy link
Contributor

sre-bot commented Apr 2, 2020

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 827edde58535ac585b4b01473a854f1d923bb54a
+++ tidb: f7fcdc25da9292b539904b4998960761faa49b57
tikv: 50397a6e9f9e4f80ac640ec2b4bf6b1315ebb9d7
pd: a22224df124e8a8f9261e4129751200255db6f2c
================================================================================
oltp_update_index:
    * QPS: 4343.46 ± 0.13% (std=4.67) delta: 0.53% (p=0.139)
    * Latency p50: 29.46 ± 0.15% (std=0.03) delta: -0.25%
    * Latency p99: 53.86 ± 3.57% (std=1.18) delta: 0.89%
            
oltp_insert:
    * QPS: 7045.55 ± 0.37% (std=19.16) delta: 1.69% (p=0.000)
    * Latency p50: 18.16 ± 0.34% (std=0.05) delta: -1.66%
    * Latency p99: 31.75 ± 1.20% (std=0.27) delta: -3.28%
            
oltp_read_write:
    * QPS: 14314.81 ± 1.49% (std=148.87) delta: -0.27% (p=0.883)
    * Latency p50: 179.19 ± 1.49% (std=1.88) delta: 0.26%
    * Latency p99: 326.09 ± 3.57% (std=8.23) delta: -2.65%
            
oltp_point_select:
    * QPS: 41337.52 ± 0.76% (std=203.87) delta: -0.31% (p=0.402)
    * Latency p50: 3.09 ± 0.81% (std=0.02) delta: 0.32%
    * Latency p99: 11.04 ± 0.00% (std=0.00) delta: 2.44%
            
oltp_update_non_index:
    * QPS: 4754.05 ± 0.34% (std=10.06) delta: 0.07% (p=0.324)
    * Latency p50: 26.92 ± 0.33% (std=0.05) delta: -0.08%
    * Latency p99: 40.86 ± 1.19% (std=0.34) delta: -0.59%
            

@coocood
Copy link
Member

coocood commented Apr 3, 2020

LGTM

@bobotu
Copy link
Contributor Author

bobotu commented Apr 3, 2020

/run-all-tests

@bobotu
Copy link
Contributor Author

bobotu commented Apr 3, 2020

/run-unit-test

@bobotu bobotu requested a review from tiancaiamao April 3, 2020 03:07
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 3, 2020
}

// SetCap sets the MemBuffer capability.
func (s *BufferStore) SetCap(cap int) {
Copy link
Member

Choose a reason for hiding this comment

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

finally, it is removed.

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

@jackysp
Copy link
Member

jackysp commented Apr 3, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 3, 2020
@jackysp jackysp removed the sig/sql-infra SIG: SQL Infra label Apr 3, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 3, 2020

/run-all-tests

@sre-bot sre-bot merged commit c81e903 into pingcap:master Apr 3, 2020
@bobotu bobotu deleted the use-sandbox branch April 3, 2020 06:55
@bobotu
Copy link
Contributor Author

bobotu commented Apr 15, 2020

/run-cherry-picker

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 15, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 15, 2020

cherry pick to release-4.0 in PR #16400

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/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants