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

tikv_alloc: update jemalloc to 5.2.1 #8324

Merged
merged 8 commits into from
Jul 31, 2020
Merged

tikv_alloc: update jemalloc to 5.2.1 #8324

merged 8 commits into from
Jul 31, 2020

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Jul 24, 2020 •
edited
Loading

What problem does this PR solve?

Problem Summary:

What is changed and how it works?

Update jemalloc to 5.2.1 to address memory allocation problem in huge page

Related changes

#4515

Check List

Tests

  • Unit test
  • Integration test

Side effects

Release note

  • update jemalloc to 5.2.1

@BusyJay
tikv_alloc: update jemalloc to 5.2.1
0af4f3a 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay BusyJay added component/util Component: tikv::util type/bugfix This PR fixes a bug. labels Jul 24, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Jul 24, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@BusyJay
Copy link
Member Author

BusyJay commented Jul 24, 2020

/release

@sre-bot
Copy link
Contributor

sre-bot commented Jul 24, 2020

download tikv at http://fileserver.pingcap.net/download/builds/pingcap/tikv/pr/0af4f3a70686e7952e9076225750dd7d206dda69/centos7/tikv-server.tar.gz

@BusyJay
fix compilation
95b999b 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
enable mem-profiling by default
cfefd9f 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
disable mem-profiling by default
37f62a0 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
Merge branch 'master' into update-jemalloc
84a4894 
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay BusyJay marked this pull request as ready for review July 29, 2020 08:44
@BusyJay
Copy link
Member Author

BusyJay commented Jul 29, 2020

@innerr You can merge to get tikv compatible with rust-rocksdb.

@brson Some changes are derived from your PR, PTAL.

@BusyJay
Copy link
Member Author

BusyJay commented Jul 29, 2020

/bench

@BusyJay
Copy link
Member Author

BusyJay commented Jul 29, 2020

/bench

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 30, 2020
@BusyJay
Copy link
Member Author

BusyJay commented Jul 30, 2020

/bench +tpcc +sysbench

sticnarf
sticnarf approved these changes Jul 30, 2020
View reviewed changes
Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 30, 2020
@BusyJay
Copy link
Member Author

BusyJay commented Jul 30, 2020

/bench +tpcc +sysbench

@sre-bot
Copy link
Contributor

sre-bot commented Jul 30, 2020

Benchmark Report

Run Sysbench Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: b16c46dba91eb05b5d60c92d185443ead8f40394
--- tikv: 990690ba71bb62365a95440a65292e37fa2ff2c3
+++ tikv: 4669c898e184d47c4761bcb01aa61d8fc904c0c6
pd: dc8c75cf7ca0395e9b5efc7ad778eae2e8620377
================================================================================
oltp_point_select:
    * QPS: 115945.08 ± 0.09% (std=82.05) delta: 0.20% (p=0.500)
    * Latency p50: 1.10 ± 0.00% (std=0.00) delta: 0.00%
    * Latency p99: 3.96 ± 0.00% (std=0.00) delta: 0.89%
            
oltp_insert:
    * QPS: 20836.09 ± 0.78% (std=111.46) delta: -0.07% (p=0.860)
    * Latency p50: 6.14 ± 0.77% (std=0.03) delta: 0.12%
    * Latency p99: 12.63 ± 0.91% (std=0.12) delta: 0.92%
            
oltp_update_non_index:
    * QPS: 25354.76 ± 0.14% (std=28.09) delta: -0.46% (p=0.002)
    * Latency p50: 5.04 ± 0.10% (std=0.00) delta: 0.40%
    * Latency p99: 10.09 ± 0.00% (std=0.00) delta: 0.00%
            
oltp_read_write:
    * QPS: 32897.29 ± 0.19% (std=47.45) delta: -0.07% (p=0.612)
    * Latency p50: 78.70 ± 0.20% (std=0.12) delta: 0.09%
    * Latency p99: 141.12 ± 0.90% (std=1.27) delta: 0.91%
            
oltp_update_index:
    * QPS: 18612.94 ± 1.99% (std=277.29) delta: -2.53% (p=0.379)
    * Latency p50: 6.85 ± 1.39% (std=0.07) delta: 2.16%
    * Latency p99: 13.34 ± 0.90% (std=0.12) delta: 0.91%

@sre-bot
Copy link
Contributor

sre-bot commented Jul 30, 2020

Benchmark Report

Run TPC-C Performance Test on VMs

@@                               Benchmark Diff                               @@
================================================================================
tidb: b16c46dba91eb05b5d60c92d185443ead8f40394
--- tikv: 990690ba71bb62365a95440a65292e37fa2ff2c3
+++ tikv: 4669c898e184d47c4761bcb01aa61d8fc904c0c6
pd: dc8c75cf7ca0395e9b5efc7ad778eae2e8620377
================================================================================
Measured tpmC (NewOrders): 5038.89 ± 1.79% (std=63.84), delta: 38.13% (p=0.220)

@brson
Copy link
Contributor

brson commented Jul 30, 2020 •
edited
Loading

Forking jemalloc doesn't strike me as a good solution no matter what the problem.

Can't this be fixed upstream?

@BusyJay
Copy link
Member Author

BusyJay commented Jul 31, 2020

Last time a PR is merged into master in upstream is the end of last year. My PRs have not been reviewed or responded for several months. Issues are not commented either. I suspect the upstream is not active anymore.

@BusyJay BusyJay added status/can-merge Indicates a PR has been approved by a committer. and removed status/DNM labels Jul 31, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@brson
Copy link
Contributor

brson commented Jul 31, 2020

OK.

@ti-srebot
Copy link
Contributor

@BusyJay merge failed.

@BusyJay BusyJay 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 Jul 31, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@BusyJay merge failed.

@BusyJay BusyJay merged commit e95e7c5 into tikv:master Jul 31, 2020
@BusyJay BusyJay deleted the update-jemalloc branch July 31, 2020 09:12
skyzh pushed a commit to skyzh/tikv that referenced this pull request Aug 4, 2020
@BusyJay @skyzh
tikv_alloc: update jemalloc to 5.2.1 (tikv#8324)
4cd8236 
Update jemalloc to 5.2.1 to address memory allocation problem in huge page.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay BusyJay added the needs-cherry-pick-release-4.0 Type: Need cherry pick to release 4.0 label Aug 19, 2020
@BusyJay
Copy link
Member Author

BusyJay commented Aug 19, 2020

/run-cherry-picker

ti-srebot pushed a commit to ti-srebot/tikv that referenced this pull request Aug 19, 2020
@ti-srebot
cherry pick tikv#8324 to release-4.0
a751f5a 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 19, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #8463

ti-srebot added a commit that referenced this pull request Sep 9, 2020
@ti-srebot
tikv_alloc: update jemalloc to 5.2.1 (#8324) (#8463)
cc44fb5 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Xintao <hunterlxt@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish approved these changes

@sticnarf sticnarf sticnarf approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

@brson brson Awaiting requested review from brson

@innerr innerr Awaiting requested review from innerr

Assignees
No one assigned
Labels
component/util Component: tikv::util needs-cherry-pick-release-4.0 Type: Need cherry pick to release 4.0 status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BusyJay @sre-bot @brson @ti-srebot @breezewish @sticnarf