Skip to content

Conversation

@djrenren
Copy link
Contributor

@djrenren djrenren commented May 7, 2021

Fixes #8252.

No idea if we're still interested in this but it was a simple change so here's the PR.

@Veykril
Copy link
Member

Veykril commented May 7, 2021

bors r+

bors bot added a commit that referenced this pull request May 7, 2021
8752: Switch from jemalloc to tikv-jemalloc r=Veykril a=djrenren

Fixes #8252.

No idea if we're still interested in this but it was a simple change so here's the PR.

Co-authored-by: John Renner <john@jrenner.net>
@lnicola
Copy link
Member

lnicola commented May 7, 2021

I think the benchmark linked in that issue used a sized deallocation variant, but I'm not sure how to set it up.

@lnicola
Copy link
Member

lnicola commented May 7, 2021

bors r-

Don't we also have to change this? https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/rust-analyzer/src/bin/main.rs#L27

@bors
Copy link
Contributor

bors bot commented May 7, 2021

Canceled.

@Veykril
Copy link
Member

Veykril commented May 7, 2021

Oh ye, we do

@djrenren
Copy link
Contributor Author

djrenren commented May 7, 2021

Whoops thought the jemalloc feature was on by default so when I saw it built I figured it was good. I'll push a fix.

@lnicola
Copy link
Member

lnicola commented May 7, 2021

bors d+

@bors
Copy link
Contributor

bors bot commented May 7, 2021

✌️ djrenren can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@djrenren
Copy link
Contributor Author

djrenren commented May 7, 2021

Okay fix is pushed. I did it by overriding the package name for jemallocator and jemalloc-ctl rather than changing the source to reference tikv_jemalloc. If that goes against any style rules or whatever, I can change it but since the two are supposed to be compatible (and presumably someday the main jemalloc crate may get updated) it seemed like a decent call. Happy to do whatever you all think is best.

@matklad
Copy link
Contributor

matklad commented May 7, 2021

I like the renaming!

bors r+

@lnicola
Copy link
Member

lnicola commented May 7, 2021

No, it's fine, I actually wanted to suggest doing that.

bors r+

@bors
Copy link
Contributor

bors bot commented May 7, 2021

Already running a review

@bors
Copy link
Contributor

bors bot commented May 7, 2021

@bors bors bot merged commit ba86203 into rust-lang:master May 7, 2021
@djrenren
Copy link
Contributor Author

djrenren commented May 7, 2021

I think the benchmark linked in that issue used a sized deallocation variant, but I'm not sure how to set it up.

As far as I can tell, tikv_jemalloc just uses sized deallocation whereas the old jemalloc does not. The original rustc PR doesn't include any configuration to turn it on and just links to the lib.rs file in tikv_jemalloctor

@lnicola
Copy link
Member

lnicola commented May 7, 2021

Ah, that's nice. I remember seeing some linker magic, but hopefully it's not needed.

@Veykril Veykril changed the title Switch from jemalloc to tikv-jemalloc minor: Switch from jemalloc to tikv-jemalloc May 8, 2021
@matklad
Copy link
Contributor

matklad commented Sep 1, 2021

For posterity, some ra benches:

Old

Database loaded:     349.36ms, 268minstr
  crates: 34, mods: 659, decls: 12785, fns: 10090
Item Collection:     6.27s, 73ginstr
  exprs: 276279, ??ty: 6712 (2%), ?ty: 3169 (1%), !ty: 1062
Inference:           14.28s, 114ginstr
Total:               20.55s, 188ginstr

New 

Database loaded:     346.65ms, 261minstr
  crates: 34, mods: 659, decls: 12785, fns: 10090
Item Collection:     6.07s, 66ginstr
  exprs: 276279, ??ty: 6712 (2%), ?ty: 3169 (1%), !ty: 1062
Inference:           14.11s, 107ginstr
Total:               20.17s, 174ginstr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Switch to tikv-jemalloc

4 participants