Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

syncer: improve performance #594

Merged
merged 14 commits into from Apr 12, 2020
Merged

syncer: improve performance #594

merged 14 commits into from Apr 12, 2020

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Apr 10, 2020 •
edited

What problem does this PR solve?

  • add metrics to diagnose perf issue
  • improve perf

What is changed and how it works?

  • add some metrics
  • increase job queue length
  • use strings.Builder to replace some fmt.Sprintf
    • strings.Builder/bytes.Buffer with sync.Pool doesn't achieve better perf
  • remove some unnecessary slice create
    • we can do more here, but it will make the code ugly and may not improve a lot, so I revert it

a sysbench workload in my laptop with the black hole downstream

transform binlog event duration (.90):

before this PR: 143us --> after this PR: 72us

binlog event QPS:

before this PR: ~4k --> after this PR: ~6k

before this PR:
image

after this PR:
image

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@csuzhangxc
*: add metric for addJob; increase job queue length and make it confi…
a803a6f 
…gurable; make flush checkpoint interval configurable
IANTHEREAL
IANTHEREAL previously approved these changes Apr 10, 2020
View reviewed changes
@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer type/enhancement Performance improvement or refactoring labels Apr 10, 2020
@csuzhangxc
Copy link
Member Author

/run-all-tests

@codecov
Copy link

codecov bot commented Apr 11, 2020 •
edited

Codecov Report

Merging #594 into release-1.0 will decrease coverage by 0.0146%.
The diff coverage is 85.2564%.

@@                 Coverage Diff                 @@
##           release-1.0       #594        +/-   ##
===================================================
- Coverage      57.2899%   57.2752%   -0.0147%     
===================================================
  Files              161        161                
  Lines            16708      16721        +13     
===================================================
+ Hits              9572       9577         +5     
- Misses            6191       6200         +9     
+ Partials           945        944         -1

@csuzhangxc
Copy link
Member Author

@GregoryIan
we have a bug before 107ef78 for transform binlog event duration with prometheus.ExponentialBuckets(0.0005, 2, 18), it will have a duration always > 450us.

In this PR, for a workload in my laptop the real transform binlog event duration decreases from ~310us to ~145us.

@csuzhangxc
Copy link
Member Author

/run-all-test

@csuzhangxc
Copy link
Member Author

/run-all-tests

@csuzhangxc csuzhangxc changed the title *: add metric for addJob; increase job queue length syncer: improve performance Apr 12, 2020
lichunzhu
lichunzhu reviewed Apr 12, 2020
View reviewed changes
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

WangXiangUSTC
WangXiangUSTC reviewed Apr 12, 2020
View reviewed changes
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

rest LGTM

dm/config/task.go Show resolved Hide resolved
WangXiangUSTC
WangXiangUSTC approved these changes Apr 12, 2020
View reviewed changes
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@WangXiangUSTC WangXiangUSTC added the status/LGT2 Two reviewers already commented LGTM, ready for merge label Apr 12, 2020
@csuzhangxc csuzhangxc merged commit d79bfc9 into pingcap:release-1.0 Apr 12, 2020
@csuzhangxc csuzhangxc deleted the more-metrics branch April 12, 2020 12:20
@csuzhangxc csuzhangxc mentioned this pull request Apr 12, 2020
Closed
csuzhangxc added a commit to csuzhangxc/dm that referenced this pull request Apr 15, 2020
@csuzhangxc
syncer: improve performance (pingcap#594)
795e3ea
@csuzhangxc csuzhangxc mentioned this pull request Apr 15, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lichunzhu lichunzhu lichunzhu left review comments

@WangXiangUSTC WangXiangUSTC WangXiangUSTC approved these changes

@IANTHEREAL IANTHEREAL IANTHEREAL left review comments

Assignees
No one assigned
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/enhancement Performance improvement or refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@csuzhangxc @WangXiangUSTC @IANTHEREAL @lichunzhu