Skip to content

[Minor perf] Avoid unnecessary allocations #14509

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Mar 31, 2025

Commits:

  1. Reserve store.late_passes with 320 and store.early_passes with 64, this leaves us some leeway for adding new passes. Note that there are not as many passes are there lints.
  2. Add [env] with some MALLOC_CONF for mainly for faster testing, but to also optimize in profiling.

This PR makes it so we avoid unnecessary reallocations, oh and we now use MALLOC_CONF for some heap-allocation optimization (I tested manually every config flag and came to this conclusion, see jemalloc/TUNING.md). I'm not sure if this would impact on rustup-distributed binaries, but I'm also taking some measures to make sure that rustup-distributed Clippy binaries (and the Rust compiler overall)) use all of Jemalloc

The performance gains vary depending on factors outside of the users control, but in wasmi (my favourite crate to benchmark due to the 66K LOC) it varies between 100ms to 400ms. Overall a solid optimization.

How to test:

There are two ways to benchmark, either with cargo lintcheck --perf in the checkout and in master, then perf diff perf.data perf.data.0, or with RUSTFLAGS="-Zself-profile" cargo dev lint <my_crate>, and measureme. Choose whichever one is most comfortable.

changelog: Minor allocation performance improvements.

@rustbot
Copy link
Collaborator

rustbot commented Mar 31, 2025

r? @Jarcho

rustbot has assigned @Jarcho.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 31, 2025
@blyxyas blyxyas added the performance-project For issues and PRs related to the Clippy Performance Project label Apr 2, 2025
@Jarcho
Copy link
Contributor

Jarcho commented May 28, 2025

r? @flip1995

I don't know how the malloc config interacts with builds in the rustc repo.

@rustbot rustbot assigned flip1995 and unassigned Jarcho May 28, 2025
@flip1995
Copy link
Member

flip1995 commented May 28, 2025

I have no idea who to ask about the effects of using MALLOC_CONF 🤷 t-bootstrap, t-compiler, t-infra, t-release? I would think t-bootstrap? Or maybe t-compiler/performance? I'd like to get some input from people closer to the compiler before merging this.

@Kobzol
Copy link
Contributor

Kobzol commented Jun 10, 2025

So Clippy doesn't use jemalloc at the moment, so for the MALLOC_CONF, we should first actually switch to it 😆 The preallocation makes sense, I suppose, and could be landed separately.

@flip1995
Copy link
Member

flip1995 commented Jun 10, 2025

@blyxyas Should we remove the MALLOC_CONF from this PR, merge the pre-allocation and revisit the MALLOC_CONF once/if we switched to jemalloc?

@Kobzol
Copy link
Contributor

Kobzol commented Jun 10, 2025

Oh, actually I didn't realize that the MALLOC_CONF thing is configured for Cargo (I thought it's configured for Clippy invocations themselves). This means that the environment variable might speed up the compilation of Clippy itself, since the host rustc probably does use jemalloc. So that makes sense.

@blyxyas
Copy link
Member Author

blyxyas commented Jun 10, 2025

Yeah, the reserves are only to avoid some reallocation we do on every Clippy run, it was just something nice to have. This PR is mainly to speed up compiling Clippy itself for cargo uitest and such.

@flip1995
Copy link
Member

Ah, I see. I misunderstood this then. Let me try on the next sync, if this affects building Clippy in the Rust repo in any way, and if it doesn't, merge this as-is.

Comment on lines +27 to +29
[env]
MALLOC_CONF = "percpu_arena:phycpu,metadata_thp:always,dirty_decay_ms:300,muzzy_decay_ms:300"
Copy link
Member

Choose a reason for hiding this comment

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

Ideally add a comment here explaining that this is for building Clippy a bit faster

1. Reserve `store.late_passes` with 320 and `store.early_passes` with 64, this leaves us some leeway for adding new passes.
2. Add [env] with some MALLOC_CONF for mainly for faster testing, but to also optimize in profiling.

;Add comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance-project For issues and PRs related to the Clippy Performance Project S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants