-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
tables: disable UpdateDeltaForTable if TxnCtx is nil #15047
tables: disable UpdateDeltaForTable if TxnCtx is nil #15047
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15047 +/- ##
===========================================
Coverage ? 80.2746%
===========================================
Files ? 502
Lines ? 131506
Branches ? 0
===========================================
Hits ? 105566
Misses ? 17570
Partials ? 8370 |
PTAL @bobotu cc @3pointer @siddontang |
Cool! LGTM |
table/tables/tables.go
Outdated
size, err := codec.EstimateValueSize(sc, r[id]) | ||
if err != nil { | ||
continue | ||
if sessVars.TxnCtx != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL @coocood
LGTM |
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. |
/run-all-tests |
@AilinKid something's wrong with the unit test.
|
OK, I will check it. |
/run-unit-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
Your auto merge job has been accepted, waiting for 15004, 14966 |
/run-all-tests |
/merge |
Your auto merge job has been accepted, waiting for 14877 |
/run-all-tests |
@kennytm merge failed. |
|
/run-all-tests |
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 entireAddRecord()
call. The delta info is useless for Lightning, so we need a way to disable it.Flamegraph for Lightning before this PR
What is changed and how it works?
Lightning will set
sessVar.TxnCtx = nil
as a signal thatUpdateDeltaForTable
is not needed. This PR adds thenil
check inAddRecord()
for skipping the computation. (UpdateRecord()
andRemoveRecord()
are not used by Lightning and thus their use ofUpdateDeltaForTable
are untouched.)Flamegraph for Lightning after this PR
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
Code changes
Side effects
Related changes
Release note