Skip to content

Revert "util: Simplify non-copy string-[]byte conversion"#11860

Merged
sre-bot merged 5 commits intopingcap:masterfrom
suzaku:revert-hack-string
Sep 17, 2019
Merged

Revert "util: Simplify non-copy string-[]byte conversion"#11860
sre-bot merged 5 commits intopingcap:masterfrom
suzaku:revert-hack-string

Conversation

@suzaku
Copy link
Contributor

@suzaku suzaku commented Aug 26, 2019

This reverts commit e954a5a.

What problem does this PR solve?

Commit e954a5a12239a1b8dab01d68f89f1270b08762a5 causes a reproducible performance
regression for the case insert_batch.

The original implementation would return directly if the input is empty,
so we try to fix this problem by adding the shortcut for empty inputs in
#11828.

But it turns out the regression is still there.

What is changed and how it works?

Revert the change.

Check List

Tests

  • Unit test

Code changes

Side effects

Related changes

@codecov
Copy link

codecov bot commented Aug 26, 2019 •
edited
Loading

Codecov Report

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

@@               Coverage Diff                @@
##             master     #11860        +/-   ##
================================================
+ Coverage   81.1224%   81.7801%   +0.6576%     
================================================
  Files           454        453         -1     
  Lines         98535      98848       +313     
================================================
+ Hits          79934      80838       +904     
+ Misses        12843      12379       -464     
+ Partials       5758       5631       -127

winoros
winoros reviewed Aug 28, 2019
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

could we add a benchmark test for the case you mentioned?

@mahjonp
Copy link
Contributor

mahjonp commented Aug 28, 2019

could we add a benchmark test for the case you mentioned?

We have a batch insert benchmark, and you can review it on perf.pingcap.net

SunRunAway
SunRunAway reviewed Aug 28, 2019
View reviewed changes
util/hack/hack.go
// Use it at your own risk.
func String(b []byte) MutableString {
return *(*MutableString)(unsafe.Pointer(&b))
func String(b []byte) (s MutableString) {
Copy link
Contributor

@SunRunAway SunRunAway Aug 28, 2019

Choose a reason for hiding this comment

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

Can you provide a benchmark for this function and provide comparable results before and after.

@mahjonp
Copy link
Contributor

mahjonp commented Aug 29, 2019

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Aug 29, 2019 

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 3de23f0f7c85b0338dd39d8318d1e4ffc75f04f3
+++ tidb: 69bfc26dfc40b75fe93dddcab0918ab18acea5d3
tikv: d3a800f30405c30d3eb3138987228829a55d52f7
pd: 14b91e8fbe3c172adcae880e3b858379e3d90eff
================================================================================
test-1: < oltp_insert >
    * QPS : 21443.48 ± 0.2482% (std=40.36) delta: -0.26%
    * AvgMs : 11.93 ± 0.2682% (std=0.02) delta: 0.25%
    * PercentileMs99 : 42.77 ± 2.1510% (std=0.58) delta: 0.38%
            
test-2: < oltp_update_non_index >
    * QPS : 29189.86 ± 0.4338% (std=77.34) delta: -1.53%
    * AvgMs : 8.77 ± 0.4334% (std=0.02) delta: 1.53%
    * PercentileMs99 : 31.03 ± 1.0827% (std=0.27) delta: 1.45%
            
test-3: < oltp_read_write >
    * QPS : 33261.37 ± 0.1679% (std=36.09) delta: 0.46%
    * AvgMs : 154.63 ± 0.1759% (std=0.17) delta: -0.45%
    * PercentileMs99 : 287.38 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 76184.75 ± 1.0472% (std=570.05) delta: -0.05%
    * AvgMs : 3.36 ± 1.1316% (std=0.03) delta: 0.01%
    * PercentileMs99 : 7.30 ± 0.0000% (std=0.00) delta: -0.71%
            
test-5: < oltp_update_index >
    * QPS : 17106.97 ± 0.3416% (std=35.34) delta: 0.14%
    * AvgMs : 14.96 ± 0.3475% (std=0.03) delta: -0.14%
    * PercentileMs99 : 48.34 ± 0.0000% (std=0.00) delta: 0.00%

http://perf.pingcap.net

@mahjonp
Copy link
Contributor

mahjonp commented Aug 29, 2019

@suzaku maybe we should update branch and benchmark again

lonng
lonng reviewed Aug 30, 2019
View reviewed changes
Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@mahjonp
Copy link
Contributor

mahjonp commented Aug 30, 2019

/bench

@sre-bot
Copy link
Contributor

sre-bot commented Aug 30, 2019 

@@                               Benchmark Diff                               @@
================================================================================
--- tidb: 3de23f0f7c85b0338dd39d8318d1e4ffc75f04f3
+++ tidb: f36fff303191a32d553659a09efa2da7664765a7
tikv: d3a800f30405c30d3eb3138987228829a55d52f7
pd: 14b91e8fbe3c172adcae880e3b858379e3d90eff
================================================================================
test-1: < oltp_insert >
    * QPS : 21583.11 ± 0.4220% (std=63.57) delta: 0.39%
    * AvgMs : 11.86 ± 0.4555% (std=0.04) delta: -0.39%
    * PercentileMs99 : 42.31 ± 1.0779% (std=0.37) delta: -0.71%
            
test-2: < oltp_update_non_index >
    * QPS : 29615.82 ± 0.0270% (std=5.66) delta: -0.09%
    * AvgMs : 8.64 ± 0.0000% (std=0.00) delta: 0.05%
    * PercentileMs99 : 30.26 ± 0.0000% (std=0.00) delta: -1.08%
            
test-3: < oltp_read_write >
    * QPS : 33397.57 ± 0.4986% (std=106.28) delta: 0.87%
    * AvgMs : 154.00 ± 0.4935% (std=0.49) delta: -0.86%
    * PercentileMs99 : 287.38 ± 0.0000% (std=0.00) delta: 0.00%
            
test-4: < oltp_point_select >
    * QPS : 75895.75 ± 1.5588% (std=663.44) delta: -0.43%
    * AvgMs : 3.36 ± 0.6701% (std=0.01) delta: 0.00%
    * PercentileMs99 : 7.38 ± 1.0572% (std=0.06) delta: 0.35%
            
test-5: < oltp_update_index >
    * QPS : 17159.77 ± 0.5823% (std=77.55) delta: 0.45%
    * AvgMs : 14.92 ± 0.5899% (std=0.07) delta: -0.43%
    * PercentileMs99 : 47.82 ± 1.0916% (std=0.43) delta: -1.08%

http://perf.pingcap.net

@zz-jason
Copy link
Member

zz-jason commented Sep 10, 2019

@suzaku Can we close this PR? Seems it's not help with the performance?

@suzaku
Copy link
Contributor Author

suzaku commented Sep 10, 2019

@mahjonp According to the benchmark result, reverting the change doesn't seem to fix the performance regression?

@mahjonp
Copy link
Contributor

mahjonp commented Sep 10, 2019

@zz-jason sorry, I will bench with batch_insert workload later, please wait a moment.

@mahjonp
Copy link
Contributor

mahjonp commented Sep 11, 2019 

Version:
tidb: f36fff303191a32d553659a09efa2da7664765a7(HEAD) 2019-08-30 12:48:23
tikv: d2e3b6af4226b17595e361da93ffcb48b89e7ec5(master) 2019-09-11 03:12:39
pd: e678a1f5c022fed729fb79397fe02b6c9f54ff4a(master) 2019-09-10 06:57:15

tidb_max_cpu: 21.63
tikv_max_cpu: 8.60
tidb_max_memory: 5160.36 MiB
tikv_max_memory: 9499.29 MiB

tps: 469.27 ± 1.4712% (std=3.81)
qps: 469.27 ± 1.4712% (std=3.81)
lat_avg: 545.37 ± 1.4601% (std=4.43)
lat_99th: 1170.73 ± 1.8096% (std=14.05)

vs

Version:
tidb: 0f2434660c509ec0b493d8c7ee0bbbf8e8549184(master) 2019-09-09 11:33:43
tikv: ff82aa9eba331585aec1c6cdf9e1584512bccb34(master) 2019-09-09 08:37:54
pd: c7c572a7dc9710aca57cb0bddf4d9bca6c4a111b(master) 2019-09-09 09:30:29

tidb_max_cpu: 22.53
tikv_max_cpu: 7.59
tidb_max_memory: 5373.36 MiB
tikv_max_memory: 7343.73 MiB

tps: 403.83 ± 2.4315% (std=5.73)
qps: 403.83 ± 2.4315% (std=5.73)
lat_avg: 633.83 ± 2.3937% (std=8.95)
lat_99th: 1412.98 ± 2.5748% (std=17.34)

there is batch insert benchmark result, @zz-jason PTAL

@mahjonp
Copy link
Contributor

mahjonp commented Sep 11, 2019

Although we haven't found the reason why e954a5a caused a performance regression, but we can ensure that this PR will not impact sysbench benchmark result, so can we merge it first? @zz-jason

zz-jason
zz-jason reviewed Sep 11, 2019
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement. labels Sep 11, 2019
@zz-jason zz-jason requested a review from SunRunAway September 12, 2019 02:12
SunRunAway
SunRunAway approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@SunRunAway SunRunAway left a comment

Choose a reason for hiding this comment

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

LGTM

@SunRunAway SunRunAway added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 17, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Sep 17, 2019

/run-all-tests

@sre-bot sre-bot merged commit b80fd0c into pingcap:master Sep 17, 2019
@suzaku suzaku deleted the revert-hack-string branch November 25, 2019 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zz-jason zz-jason zz-jason left review comments

@winoros winoros winoros left review comments

+2 more reviewers

@lonng lonng lonng left review comments

@SunRunAway SunRunAway SunRunAway approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@suzaku @mahjonp @sre-bot @zz-jason @SunRunAway @lonng @winoros