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 mimalloc feature to tikv_alloc #5041

Merged
merged 9 commits into from Jul 11, 2019

Conversation

@sticnarf
Copy link
Contributor

commented Jul 7, 2019

Signed-off-by: Yilin Chen sticnarf@gmail.com

What have you changed? (mandatory)

This PR adds mimalloc support to TiKV. To use mimalloc as the allocator of TiKV, set MIMALLOC environment variable when building TiKV, like:

MIMALLOC=1 make dist_release

The mimalloc crate used is https://github.com/gnzlbg/mimallocator whose maintainer is the same as jemallocator's.

What are the type of the changes? (mandatory)

  • New feature (change which adds functionality

How has this PR been tested? (mandatory)

Untested

Does this PR affect documentation (docs) or release note? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

A little improvement under sysbench while a little regression under go-ycsb. It should be investigated more.

image
image

Add a few positive/negative examples (optional)

Add mimalloc feature to tikv_alloc
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

Great work!!!

Do we statically link mimalloc?

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

does mimalloc support outputting statistics like jemalloc?

@kennytm

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2019

You could enable the stats feature of the mimalloc dependency to print the stats on exit.


[dependencies.mimallocator]
version = "0.1.3"
optional = true

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 7, 2019

Member

trailing new line

Enable stats feature of mimalloc
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Do we statically link mimalloc?

Yes. I believe it is statically linked.

@sticnarf

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Now, stats feature is enabled.

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

do we need to implement fetch_stats and dump_stats?

@sticnarf

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@siddontang The stats feature just prints statistics to stderr when the program exits.
The mimallocator crate does not support runtime statistics yet but it is possible to call mi_stats_print to collect statistics.

@sticnarf

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

After a second test, I am confident it's not a mistake that sysbench and go-ycsb give different results.

I think it just tells us mimalloc does not work better in every case. When value size differs, allocation size and frequency can differ too. No allocator can handle every case well.

@kennytm

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

(Just to clarify I don't mean the stats feature should always be enabled 😅)

@breeswish

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

Maybe related: according to microsoft/mimalloc#11 mimallocator was not built for many short lived small allocations.

sticnarf added some commits Jul 8, 2019

Don't enable stats feature of mimalloc
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

(Just to clarify I don't mean the stats feature should always be enabled )

Sorry, at first I thought the stats feature of mimalloc is similar with that of jemalloc 😅

@siddontang
Copy link
Contributor

left a comment

I am fine with this now, I think mimalloc can be improved better soon, then we can do another benchmark again.

LGTM

PTAL @breeswish @brson

kennytm and others added some commits Jul 9, 2019

@siddontang

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

CI failed @sticnarf

@kennytm kennytm merged commit f2ad60a into tikv:master Jul 11, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.