Remove itertools dependency from rustc_ast_pretty

[blyxyas]

3 Second optimization in the rustc_ast_pretty module compiletimes (13.57s -> 10.58s)! Not much, but it's honest work :)
Found by an experiment I'm doing, related with Incremental Systems Rethought project goal.

So, cc rust-lang/goals#641

r? @jackh726

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[mejrs]

Personally I don't think the difference looks good, it screams "hey, itertools has something for this", someone will do it, and then we're right back where we started.

3 Second optimization in the rustc_ast_pretty module compiletimes (13.57s -> 10.58s)!

Can you clarify what exactly is measured here? This is totally different from what I'm seeing.

For reference, this is what my build timings look like:

• 0s: start x.py build
• 8s: itertools starts compiling
• 13s: itertools finishes compiling
• 37s: rustc_ast_pretty starts compiling
• 39s: rustc_ast_pretty finishes compiling
• 210s: rustc_driver finishes compiling

Furthermore itertools is used later (rustc_trait_selection and rustc_passes are up next, iirc) so it's not like it is dropped completely.

[mejrs]

r? me

As per ^ the "optimization" argument doesn't make much sense to me, can you clarify? Maybe there's something I'm missing or misunderstanding.

That said I also think the case for using itertools here is weak, so happy to drop it regardless.

@bors r+ rollup

View changes since this review

[rust-bors]

📌 Commit 68ff3f2 has been approved by mejrs

It is now in the queue for this repository.

[Zalathar]

Yeah, I am also not convinced by the build-time argument, but I think these particular uses of itertools are net-negative for clarity anyway, and being able to remove itertools from this crate is a nice bonus.

[blyxyas]

Can you clarify what exactly is measured here? This is totally different from what I'm seeing.

The time measurement is just by doing x clean && x b rustc_ast_pretty with both versions.
For a more empirical evidence that this is a good change, we can look at the depgraph, extracted with the following command:

cargo depgraph --focus itertools > graph-itertools.dot
git switch reduce-deps
cargo depgraph --focus itertools > graph-itertools-reduced.dot

Comparing the itertools one, we find 53 nodes in the current main, while only 35 dependencies on the reduced version. Meaning that there were 18 dependants or dependencies that needed itertools explicitly because rustc_ast_pretty needed it. (Likely because they depended on rustc_ast).

So, the list of crates that exhibit performance improvements when built individually can be extracted from that graph:

• rustc_attr_parsing
• rustc_ast_pretty
• rustc_hir
• rustc_parse
• rustc_session
• rustc_incremental
• rustc_middle
• rustc_hir_pretty
• rustc_infer
• rustc_mir_dataflow
• rustc_transmute
• rustc_symbol_mangling
• rustc_ast_lowering
• rustc_pattern_analysis
• rustc_monomorphize
• rustc_query_impl
• rustc_public
• rustc_public_bridge

[mejrs]

That doesn't justify the "I shaved 3 seconds off compilation time" rhetoric.

The time measurement is just by doing x clean && x b rustc_ast_pretty with both versions.

So this is just the improvement of not building itertools here. It's still used in other parts of the compiler so all you've shown is that when you move around dependencies to another part of the crate graph then this part of the graph compiles quicker. This is likely only meaningful if you (please don't do this) removed the itertools dependencies everywhere in the compiler.

But even making x clean && x b rustc_ast_pretty faster (which on its face sounds like it might be useful for some contributors) is ultimately a meaningless metric because ~nobody does this (clean their artifacts and build only a particular rustc crate).

For the sake of argument, say they're working on rustc_ast_pretty. They'll likely build the entire compiler first (which this change doesn't matter for), and maybe run some tests to check that everything is OK. After that it is irrelevant what rustc_ast_pretty depends on, because making changes to rustc_ast_pretty doesn't require recompiling its dependencies.

Anyway, I'd be interested to see your timing graphs with and without this change. For reference, this is mine: https://github.com/user-attachments/files/30619595/cargo-timing-clean%2Brustc_ast_pretty.html

(as you can see, for my particular system, itertools is not bottlenecking anything and sits idle for ~10 seconds before actually being needed, so probably even this change on this particular benchmark does little for me)

[blyxyas]

For the sake of argument, say they're working on rustc_ast_pretty

This is an improvement if they work on rustc_ast_pretty only, or on one of the other 17 nodes I listed earlier. If we were only to build rustc_ast, the same effect applies. Same with rustc_public or even rustc_middle.

This also applies to other scenarios where a lot of things change at the same time, like changing Git branches or modifying bootstrap.toml.

Here are my timings: timings-clean+x_b_rustc_ast_pretty.html

itertools is still the 5th biggest dependency in time (1.22s) and 5th in size itself (6.5MB).

---

If you don't think that the optimization has merit, I can agree. At the end of the day it was me trying out if I could find something cool with some metadata analysis of the compiler.

[panstromek]

Even if the build time argument is weak, this still means that the full build will use slightly less resources overall because at least one less crate will have to load and process itertools dependency data (not sure about rustc_ast_pretty dependants in this case). I'm myself currently bottlenecked on memory and can't even use all cores when building rustc, so I welcome any improvements in this area, even though this one is probably tiny in grand scheme of things.

We have the result from a rollup, which seems mildly positive on boostrap but below noise treshold and sadly there's a massive regression from other PR in that rollup.

But the queue is empty and we have unrolled build, so we can check the effect on boostrap in isolation, I'm curious:

@rust-timer build a0632a7

[rust-timer]

Finished benchmarking commit (a0632a7): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.2%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.2%	[0.4%, 4.1%]	5
Regressions ❌ (secondary)	0.5%	[0.4%, 0.5%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[0.4%, 4.1%]	5

Cycles

Results (primary 0.7%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.2%	[0.4%, 2.4%]	10
Regressions ❌ (secondary)	1.4%	[0.4%, 3.9%]	8
Improvements ✅ (primary)	-0.7%	[-0.9%, -0.4%]	3
Improvements ✅ (secondary)	-0.7%	[-0.9%, -0.4%]	6
All ❌✅ (primary)	0.7%	[-0.9%, 2.4%]	13

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.834s -> 497.836s (1.43%)
Artifact size: 390.52 MiB -> 390.55 MiB (0.01%)

[panstromek]

Nice. We hit the infamous 7s noise spike on bootstrap, so this run also doesn't tell us much 😄 But rustc_ast_pretty shows a tiny improvement in both runs so that's at least some weak signal.