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.

Sign up for GitHub

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

Jump to bottom

tables: disable UpdateDeltaForTable if TxnCtx is nil #15047

Merged
merged 7 commits into from
Mar 3, 2020
Merged

tables: disable UpdateDeltaForTable if TxnCtx is nil #15047

merged 7 commits into from
Mar 3, 2020

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Feb 29, 2020 •
edited
Loading

What problem does this PR solve?

This is a follow-up to pingcap/tidb-lightning#274.

In the flamegraph of AddRecord() (with a special session used by Lightning), it was found that calculation used for (*TransactionContext).UpdateDeltaForTable() occupy 27% run time (!) of the entire AddRecord() call. The delta info is useless for Lightning, so we need a way to disable it.

Flamegraph for Lightning before this PR

Annotation 2020-03-01 033907-fs8

What is changed and how it works?

Lightning will set sessVar.TxnCtx = nil as a signal that UpdateDeltaForTable is not needed. This PR adds the nil check in AddRecord() for skipping the computation. (UpdateRecord() and RemoveRecord() are not used by Lightning and thus their use of UpdateDeltaForTable are untouched.)

Flamegraph for Lightning after this PR

Annotation 2020-03-01 034115-fs8

The time needed to encode one row of TPC-C "CUSTOMER" table (21 columns, 1 index) went down from 10.5 ms/op to 8.2 ms/op.

Note that this PR does not affect TiDB itself, since sessVar.TxnCtx should always be non-nil in normal use. Thus no test cases were added in TiDB itself. (Test for correctness will be in Lightning.)

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

Release note

@codecov
Copy link

codecov bot commented Feb 29, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@5c1ee62). Click here to learn what that means.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #15047   +/-   ##
===========================================
  Coverage          ?   80.2746%           
===========================================
  Files             ?        502           
  Lines             ?     131506           
  Branches          ?          0           
===========================================
  Hits              ?     105566           
  Misses            ?      17570           
  Partials          ?       8370

@kennytm
Copy link
Contributor Author

kennytm commented Feb 29, 2020

PTAL @bobotu cc @3pointer @siddontang

@IANTHEREAL
Copy link
Contributor

Cool! LGTM

@IANTHEREAL
Copy link
Contributor

@bobotu @lysu PTAL

@IANTHEREAL IANTHEREAL added status/LGT1 Indicates that a PR has LGTM 1. and removed status/PTAL labels Mar 1, 2020
siddontang
siddontang reviewed Mar 1, 2020
View reviewed changes
table/tables/tables.go Outdated
size, err := codec.EstimateValueSize(sc, r[id])
if err != nil {
continue
if sessVars.TxnCtx != nil {
Copy link
Member

Choose a reason for hiding this comment

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

PTAL @coocood

@bobotu
Copy link
Contributor

bobotu commented Mar 1, 2020

LGTM

@bobotu bobotu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 1, 2020
@kennytm
Copy link
Contributor Author

kennytm commented Mar 1, 2020 •
edited
Loading

In the 33 GB TPC-C "CUSTOMER" CSV table test, the time needed to Encode() one 256 MiB chunk is reduced from ~28.5s to ~24.5s (86%), matching the expectation (Encode() doesn't just call AddRecord(), it also calls Convert(), so the ratio is higher than 73%).

However, the entire CSV → Importer step is only slightly improved from 2m37s to 2m34s (98%). This is because Encode() speed has caught up with the KV → Importer transfer speed, and is no longer the bottleneck.

Nevertheless, since the Encode() time is really decreased, I still think this PR is worth for Lightning for other situations where Encode() might still be the bottleneck.

coocood
coocood reviewed Mar 2, 2020
View reviewed changes
table/tables/tables.go Outdated Show resolved Hide resolved
@zimulala
Copy link
Contributor

zimulala commented Mar 2, 2020

/run-all-tests

@kennytm
Copy link
Contributor Author

kennytm commented Mar 2, 2020

@AilinKid something's wrong with the unit test.

[2020-03-02T07:09:15.193Z] FAIL: binloginfo_test.go:380: testBinlogSuite.TestBinlogForSequence
[2020-03-02T07:09:15.193Z] 
[2020-03-02T07:09:15.193Z] binloginfo_test.go:436:
[2020-03-02T07:09:15.193Z]     c.Assert(mustGetDDLBinlog(s, "select setval(`test2`.`seq2`, 6)", c), IsTrue)
[2020-03-02T07:09:15.193Z] ... obtained bool = false

@AilinKid
Copy link
Contributor

AilinKid commented Mar 2, 2020

@AilinKid something's wrong with the unit test.

[2020-03-02T07:09:15.193Z] FAIL: binloginfo_test.go:380: testBinlogSuite.TestBinlogForSequence
[2020-03-02T07:09:15.193Z] 
[2020-03-02T07:09:15.193Z] binloginfo_test.go:436:
[2020-03-02T07:09:15.193Z]     c.Assert(mustGetDDLBinlog(s, "select setval(`test2`.`seq2`, 6)", c), IsTrue)
[2020-03-02T07:09:15.193Z] ... obtained bool = false

OK, I will check it.

@AilinKid
Copy link
Contributor

AilinKid commented Mar 2, 2020

/run-unit-test

coocood
coocood approved these changes Mar 2, 2020
View reviewed changes
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.

LGTM

@coocood
Copy link
Member

coocood commented Mar 2, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 2, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 2, 2020

Your auto merge job has been accepted, waiting for 15004, 14966

@sre-bot
Copy link
Contributor

sre-bot commented Mar 2, 2020

/run-all-tests

@zz-jason
Copy link
Member

zz-jason commented Mar 3, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

Your auto merge job has been accepted, waiting for 14877

@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

@kennytm merge failed.

@kennytm
Copy link
Contributor Author

kennytm commented Mar 3, 2020 •
edited
Loading 

[2020-03-03T02:29:15.145Z] FAIL: builtin_time_test.go:764: testEvaluatorSuite.TestNowAndUTCTimestamp
[2020-03-03T02:29:15.145Z] 
[2020-03-03T02:29:15.145Z] builtin_time_test.go:798:
[2020-03-03T02:29:15.145Z]     c.Assert(ts.Sub(gotime(t, ts.Location())), LessEqual, 3*time.Millisecond)
[2020-03-03T02:29:15.145Z] ... compare_one time.Duration = 7432786 ("7.432786ms")
[2020-03-03T02:29:15.145Z] ... compare_two time.Duration = 3000000 ("3ms")
[2020-03-03T02:29:15.145Z]

@kennytm kennytm added status/can-merge Indicates a PR has been approved by a committer. and removed status/can-merge Indicates a PR has been approved by a committer. labels Mar 3, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 3, 2020

/run-all-tests

@sre-bot sre-bot merged commit 495f8b7 into pingcap:master Mar 3, 2020
@kennytm kennytm deleted the disable-update-diff-in-lightning-mode branch March 3, 2020 04:16
XuHuaiyu pushed a commit to XuHuaiyu/tidb that referenced this pull request Mar 5, 2020
@kennytm @XuHuaiyu
tables: disable UpdateDeltaForTable if TxnCtx is nil (pingcap#15047)
db3a8f0
XuHuaiyu pushed a commit to XuHuaiyu/tidb that referenced this pull request Mar 5, 2020
@kennytm @XuHuaiyu
tables: disable UpdateDeltaForTable if TxnCtx is nil (pingcap#15047)
ba0c83c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siddontang siddontang siddontang left review comments

@coocood coocood coocood approved these changes

Assignees
No one assigned
Labels
component/statistics status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@kennytm @IANTHEREAL @bobotu @zimulala @AilinKid @coocood @sre-bot @zz-jason @siddontang @ngaut