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.

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

Add tcmalloc support to the tikv_alloc crate #4370

Merged
merged 37 commits into from Apr 12, 2019

Conversation

Projects
None yet
10 participants
@ZhangHanDong
Copy link
Contributor

ZhangHanDong commented Mar 13, 2019

Re-PR from #4207

Add tcmalloc support to the tikv_alloc crate for issues #4191

  • remove the no-jemalloc feature from tikv's Cargo.toml
  • change the tikv_alloc crate to default-features = false
  • add tikv_alloc/jemalloc to tikv's default features.
  • enabling tcmalloc for tikv means passing `cargo build --no-default-features --- features="tikv_alloc/tcmalloc".
Add tcmalloc support to the tikv_alloc crate
Signed-off-by: blackanger <blackanger.z@gmail.com>
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Mar 13, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

fixed unused warning : unused libc
Signed-off-by: blackanger <blackanger.z@gmail.com>
@AndreMouche

This comment has been minimized.

Copy link
Member

AndreMouche commented Mar 14, 2019

Please merge master and fix the conflicts.

@AndreMouche AndreMouche requested a review from brson Mar 14, 2019

@Hoverbear
Copy link
Member

Hoverbear left a comment

Thanks for this! A couple nits!

Show resolved Hide resolved components/tikv_alloc/src/jemalloc.rs Outdated
Show resolved Hide resolved components/tikv_alloc/src/jemalloc.rs

ZhangHanDong added some commits Mar 15, 2019

fixed typo
Signed-off-by: blackanger <blackanger.z@gmail.com>
resolved conflicts
Signed-off-by: blackanger <blackanger.z@gmail.com>
@brson

This comment has been minimized.

Copy link
Contributor

brson commented Mar 20, 2019

Hi again @ZhangHanDong!

I spent some time looking at this tonight, and it looks good to me
as-is. Nice work.

There's a bit of a problem though: there's some new jemalloc code on master
that breaks tikv_alloc for non-jemalloc builds.

I will propose a fix for that in another PR shortly, and I'll ping
you. It will cause merge conflicts for you.

Since this PR is passing tests we could merge it before that PR,
even though non-jemalloc allocators will not build. I'll think about
it overnight while we wait for another approving reviewer.

Will you be interested in doing any performance comparisons between the two allocators?

@brson
Copy link
Contributor

brson left a comment

LGTM

@kennytm kennytm added the S: LGT1 label Mar 20, 2019

@ZhangHanDong

This comment has been minimized.

Copy link
Contributor Author

ZhangHanDong commented Mar 20, 2019

@brson yeah. I can do some performance comparisons between the two allocators, but I am not sure if it can be done in macOSX, maybe I need to prepare a linux environment. And I also need you to guide me on how to do the performance testing.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Mar 20, 2019

A potential follow-up issue @ZhangHanDong: #4409

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Mar 20, 2019

Here's the PR I mentioned before to fix current breakage of a no-jemalloc build: #4411

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Mar 20, 2019

I can do some performance comparisons between the two allocators, but I am not sure if it can be done in macOSX, maybe I need to prepare a linux environment. And I also need you to guide me on how to do the performance testing.

Heh, I haven't really thought much about how to do it, but here are some ideas, from easy to hard.

  • cargo bench --no-run && time cargo bench

    • just see if there's any change to the amount of time it takes to run the entire benchmark suite
  • time the integration benchmarks

  • find the max RSS of each under a heavy workload

    • time -v tikv_server will report rss, feed it a difficult test from tidb-bench
  • compare the metrics reported by jemalloc and tcmalloc

    • tcmalloc reports metrics that are comparable to jemalloc's (see "Generic Tcmalloc Status" here
    • add an implementation of fetch_stats, run go-ycsb, and compare their reports

Those are all the ideas I've got right now.

@yiwu-arbug has been working with jemalloc recently. Maybe they have better ideas about how to compare jemalloc to tcmalloc.

Edit: measuring fragmentation under load would be very good, but I don't know how to do it.

@yiwu-arbug

This comment has been minimized.

Copy link
Contributor

yiwu-arbug commented Mar 21, 2019

I don't have any experience with tcmalloc, but to compare the stats, it seems that we can:

  • make sure jemalloc.allocated is equal to tcmalloc.current_allocated_bytes
  • make sure jemalloc.resident is comparable to tcmalloc.heap_size
@siddontang

This comment has been minimized.

Copy link
Contributor

siddontang commented Mar 26, 2019

Please fix the conflicts @ZhangHanDong

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Mar 27, 2019

I'm sorry about the delay getting back to this. It's near the top of my todo list.

@brson brson dismissed their stale review via b7d3655 Mar 28, 2019

@brson
Copy link
Contributor

brson left a comment

I pushed a commit that fixes the merge conflicts.

In the process I discovered that the tcmalloc crate doesn't build tcmalloc. It's just a small set of declarations that expect tcmalloc to exist in the linker path. Eventually that will need to change for Rust projects to use the tcmalloc crate, but it's good enough for casual testing.

I'm inclined to go ahead and merge this without measurements in the hopes that myself or someone else will circle back and do an evaluation. And maybe now that the multi-malloc structure is in place we can also experiment with other allocators.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Mar 28, 2019

I wonder who else is interested in the allocator and would review. Maybe @BusyJay ?

Resolved conflicts
Signed-off-by: blackanger <blackanger.z@gmail.com>

ZhangHanDong added some commits Apr 8, 2019

Show resolved Hide resolved Makefile Outdated

ZhangHanDong added some commits Apr 9, 2019

remove trailing spaces
Signed-off-by: blackanger <blackanger.z@gmail.com>
@brson

This comment has been minimized.

Copy link
Contributor

brson commented Apr 10, 2019

OK I left two more change requests related to the initialization of ENABLE_FEATURES and the name of the tcmalloc source file. I'm confident that once those are resolved this will be ready. Thanks @ZhangHanDong.

@@ -42,3 +39,7 @@ features = ["stats"]
[dependencies.jemalloc-ctl]
version = "0.2.0"
optional = true

[dependencies.tcmalloc]
version = "0.2.0"

This comment has been minimized.

Copy link
@siddontang

siddontang Apr 10, 2019

Contributor

can we use @brson's branch so we can link tcmalloc statically?

This comment has been minimized.

Copy link
@ZhangHanDong

ZhangHanDong Apr 10, 2019

Author Contributor

@siddontang

[dependencies.tcmalloc]
# version = "0.2.0"
optional = true
git = "https://github.com/brson/tcmalloc-rs"
features = ["bundled"]

This should work, but only try in linux OS. But It is recommended to make other attempts after merge the PR.

This comment has been minimized.

Copy link
@brson

brson Apr 11, 2019

Contributor

I'll land that one after this does.

ZhangHanDong added some commits Apr 10, 2019

modify review change
Signed-off-by: blackanger <blackanger.z@gmail.com>
add default-features=false for engine's Cargo.toml and resolve conflict
Signed-off-by: blackanger <blackanger.z@gmail.com>
@ZhangHanDong

This comment has been minimized.

Copy link
Contributor Author

ZhangHanDong commented Apr 10, 2019

@brson Already modified as required.

And I added default-features = false for engine's Cargo.toml:

tikv_alloc = { path = "../tikv_alloc", default-features = false }

ZhangHanDong added some commits Apr 10, 2019

Trigger test
Signed-off-by: blackanger <blackanger.z@gmail.com>
@brson

brson approved these changes Apr 11, 2019

@brson brson requested a review from siddontang Apr 11, 2019

@brson brson referenced this pull request Apr 11, 2019

Merged

Upgrade tcmalloc-rs to 0.3 #4516

@ice1000 ice1000 added S: LGT2 and removed S: LGT1 labels Apr 12, 2019

@brson brson merged commit 2b242b7 into tikv:master Apr 12, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
@brson

This comment has been minimized.

Copy link
Contributor

brson commented Apr 12, 2019

Woo! Thanks @ZhangHanDong !

If you are interested there is a followup issue to reorganize these allocator features: #4409

@ZhangHanDong

This comment has been minimized.

Copy link
Contributor Author

ZhangHanDong commented Apr 12, 2019

@brson Ok, let me try it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.