-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
*: Build TiDB with PGO #58630
base: master
Are you sure you want to change the base?
*: Build TiDB with PGO #58630
Conversation
Hi @crazycs520. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #58630 +/- ##
================================================
+ Coverage 73.0824% 73.5414% +0.4590%
================================================
Files 1684 1689 +5
Lines 466409 478991 +12582
================================================
+ Hits 340863 352257 +11394
- Misses 104588 105095 +507
- Partials 20958 21639 +681
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: crazycs520 <crazycs520@gmail.com>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance6716 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
/retest-required |
@crazycs520: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: crazycs520 <crazycs520@gmail.com>
We can test more workloads like using benchbot to see its overall effect. |
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
I've uploaded some test results from other tests inside the PR description. |
Signed-off-by: crazycs520 <crazycs520@gmail.com>
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.
Here are some other suggestions:
- It would be better to show the PGO info in
tidb-server -V
. - Add a README.md to
build/profile
to explain the standard way to generate those pprof files.
BTW, do we need to update the build flags for bazel as well?
default.pgo
Outdated
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.
Since this file will be generated by make, is it necessary to include it in git? IMO, we only need to keep either default.pgo
or build/profile/*.proto
.
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.
+1
@crazycs520 Better to also add some introduction for the choice of default.pgo like how it is generated and why it is chosen as default, which options are recommonded in different scenarios.
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.
default.pgo
need to keep since the build plugin will use it, otherwise I may need to change other ci/build yaml of plugin.
as build/profile/*.proto
files, I also want to keep it, since one look at this folder will tell us which CPU profiles default.pgo
is currently using.
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.
I write a test about git with large binary file, from the following test result, It wasn't really appropriate to put the profile files inside the git repo, so I've deleted them.
Modified binary file, iteration 1, size: 1 MB, dir_size: 1 MB, git cost: 2.901416ms
Modified binary file, iteration 2, size: 1 MB, dir_size: 2 MB, git cost: 1.016208ms
Modified binary file, iteration 3, size: 1 MB, dir_size: 3 MB, git cost: 1.069875ms
...
Modified binary file, iteration 1000, size: 1 MB, dir_size: 1001 MB, git cost: 35.69275ms
We can use the following command to check the PGO info of a binary.
Great, I already add a
@hawkingrei PTAL |
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Signed-off-by: crazycs520 <crazycs520@gmail.com>
Makefile
Outdated
else | ||
CGO_ENABLED=1 $(GOBUILD) -pgo=default.pgo $(RACE_FLAG) -ldflags '$(LDFLAGS) $(CHECK_FLAG)' -o '$(TARGET)' ./cmd/tidb-server | ||
endif | ||
|
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.
IMO, there is no need to update Makefile. According to the doc of PGO, go build
will detect default.pgo
files automatically and enable PGO. Thus, we only need to put a default.pgo
to cmd/tidb-server
when we want to build with PGO and this can be done in CI/CD.
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.
Great, I used to put default.pgo
in tidb
repo root dir, but it doesn't work, then I add this into Makefile. Put default.pgo
to cmd/tidb-server
is work.
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.
done.
Signed-off-by: crazycs520 <crazycs520@gmail.com>
What problem does this PR solve?
Issue Number: close #57502
Problem Summary: Build TiDB with PGO
What changed and how does it work?
This PR support builds TiDB with PGO. According to the doc of PGO, go build will detect
default.pgo
files automatically and enable PGO, so you need to putdefault.pgo
tocmd/tidb-server/
dir to enable PGO.Watch out, there's a potential performance regression risk here, see golang/go#71400
As you can see from the tests below, the performance of TiDB is improved after compiling with PGO, even with the old (v7.0.0) profile, but the improvement is less, so we need to update the
default.pgo
profile regularly in the future.Sysbench Benchmark
Topology
Benchmark from Benchbot
Following benchmark use a merged
cpu_profile
(merge profile files frombuild/profile
folder) to build TiDB with PGO. As you can see from the results, the performance of TiDB compiled with pgo is generally better.Sysbench
Sysbench on ARM CPU
TPCC
YCSB
Bank
benchmarksql
tpcds
query_28.sql,query_88.sql
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.