Skip to content

Suggest using the updated jemalloc lib #259

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

Closed
thaodt opened this issue Sep 28, 2021 · 4 comments · Fixed by #264
Closed

Suggest using the updated jemalloc lib #259

thaodt opened this issue Sep 28, 2021 · 4 comments · Fixed by #264
Assignees
Labels
dependencies Pull requests that update a dependency file

Comments

@thaodt
Copy link
Contributor

thaodt commented Sep 28, 2021

Hi team, I've just noticed that we're using the outdated jemallocator.
Check this PR from rust-core for more details.

Updated jemalloc lib
This updated one is from tikv

TL;DR: this upgrade could help for sized deallocation's usage correctly, particularly to rustdoc.
So somehow it can deliver a sizable performance increase. We can do some benchmarks after that.

Lets consider my suggestion. Thank you.

@escapedcat
Copy link
Contributor

Thanks!

You mean this?:
https://github.com/SaitoTech/saito-rust/blob/main/Cargo.toml#L34-L35
Is jemalloc-ctl still ok?

Wanna create a PR to use the suggested alternative?

@escapedcat escapedcat added the dependencies Pull requests that update a dependency file label Sep 28, 2021
@thaodt
Copy link
Contributor Author

thaodt commented Sep 28, 2021

Thanks!

You mean this?: https://github.com/SaitoTech/saito-rust/blob/main/Cargo.toml#L34-L35 Is jemalloc-ctl still ok?

Wanna create a PR to use the suggested alternative?

Sure, will do that. For jemalloc-ctl, its already included in tikv-jemallocator. Therefore, we just need to replace its usage.

@clayrab
Copy link
Contributor

clayrab commented Sep 30, 2021

Thanks so much for pointing this out, Thao.

The reason I added jemalloc in the first place was related to jemalloc-ctl because I wanted to do some introspection on memory usage in the old codebase... we aren't currently using this because the debate about memory usage has basically been settled...

If I'm understanding the discussions linked above correctly, it seems that rust-lang is already using tikv-jemallocator, so perhaps we should just remove both jemalloc packages from the toml and delete memory-stats.rs?

That being said, if this is mostly an improvement for rustdoc, I'm not particularly concerned about that unless it would improve compile times during dev... but in theory that shouldn't be the case, right?(i.e. why would be build the docs on every compile?).

I think the proper way to do this would be as Thao suggested and do some very basic benchmarks, at least. However, I wouldn't be opposed to simply removing the packages, since that's probably the right thing to be doing anyway. If no one else wants to handle this, I'll probably just proceed this way.

@thaodt
Copy link
Contributor Author

thaodt commented Sep 30, 2021

Thanks so much for pointing this out, Thao.

The reason I added jemalloc in the first place was related to jemalloc-ctl because I wanted to do some introspection on memory usage in the old codebase... we aren't currently using this because the debate about memory usage has basically been settled...

If I'm understanding the discussions linked above correctly, it seems that rust-lang is already using tikv-jemallocator, so perhaps we should just remove both jemalloc packages from the toml and delete memory-stats.rs?

That being said, if this is mostly an improvement for rustdoc, I'm not particularly concerned about that unless it would improve compile times during dev... but in theory that shouldn't be the case, right?(i.e. why would be build the docs on every compile?).

I think the proper way to do this would be as Thao suggested and do some very basic benchmarks, at least. However, I wouldn't be opposed to simply removing the packages, since that's probably the right thing to be doing anyway. If no one else wants to handle this, I'll probably just proceed this way.

Got your point, Clay. Upon checking, its only used on memory-stats.rs, so if that file is useless now, we can simply removing it & the related packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants