Store locals in borrowck in BitSets

[panstromek]

View all comments

[panstromek]

This will probably be neutral or regression, but I want to see whether there's something interesting happening and whether this is a useful starting point to bitsetivize the main part of the algorithm

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: f483b29 (f483b29900f28ae4e68d2919f720ac875fcaa313)
Base parent: c42ac5f (c42ac5fd59628ea7e2f52af5944c2aaac3f0e7f6)

[rust-timer]

Finished benchmarking commit (f483b29): comparison URL.

Overall result: ✅ improvements - 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

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.9%, secondary 2.4%)

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

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.4%, 3.4%]	2
Regressions ❌ (secondary)	2.4%	[2.2%, 2.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.9%	[2.4%, 3.4%]	2

Cycles

Results (primary -2.0%, secondary -5.2%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-5.2%	[-5.2%, -5.2%]	1
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

Binary size

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

Bootstrap: 474.678s -> 476.443s (0.37%)
Artifact size: 402.79 MiB -> 402.75 MiB (-0.01%)

[panstromek]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: adb15a3 (adb15a3dc8b1095bd6087d0490f20cfcb617de75)
Base parent: 17fd5b8 (17fd5b8a37b6667b6cc137f3cc35f09759768a3b)

[rust-timer]

Finished benchmarking commit (adb15a3): 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 (secondary -0.2%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.5%	[1.0%, 6.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-4.6%, -2.9%]	2
All ❌✅ (primary)	-	-	0

Cycles

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

Binary size

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

Bootstrap: 474.378s -> 474.623s (0.05%)
Artifact size: 402.85 MiB -> 402.87 MiB (0.01%)

[panstromek]

This result looks better than the previous one. I wonder what happened with tt-muncher, I guess the improvement in the first run was some inlining noise?

[panstromek]

This will probably conflict with #161937 and #161938.

cc @jackh726. Do you have thoughts on this? I want know whether it's worthwile to continue experiments in this direction. If you have plans to completely rewrite this code or something like that, then I should probably pause or abandon this.

[panstromek]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 7b6a9f4 (7b6a9f465e93bbe380df5c2a851e710bfe505c8e)
Base parent: 58ae2c4 (58ae2c4315128abc40be24b429dfb68bf27510b3)

[rust-timer]

Finished benchmarking commit (7b6a9f4): comparison URL.

Overall result: ❌✅ regressions and improvements - 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

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.3%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.1%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -4.0%, 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)	-	-	0
Regressions ❌ (secondary)	4.6%	[4.5%, 4.7%]	2
Improvements ✅ (primary)	-4.0%	[-5.1%, -3.0%]	2
Improvements ✅ (secondary)	-4.0%	[-5.3%, -1.8%]	3
All ❌✅ (primary)	-4.0%	[-5.1%, -3.0%]	2

Cycles

Results (primary -2.1%)

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

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Binary size

Results (primary 0.0%)

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

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	3

Bootstrap: 475.447s -> 473.197s (-0.47%)
Artifact size: 402.81 MiB -> 402.87 MiB (0.02%)

[jackh726]

This will probably conflict with #161937 and #161938.

cc @jackh726. Do you have thoughts on this? I want know whether it's worthwile to continue experiments in this direction. If you have plans to completely rewrite this code or something like that, then I should probably pause or abandon this.

This does conflict, but in only in practice, not principle. I don't expect that landing one will have a meaningful effect on the other (aside from just needing to do a simple rebase).

I'm separately trying to refactor the live loan computation to be loans-per-point, rather than points-per-loan - which I think will conflict more. (Though, if you're in the area and want to work on that, then that's okay.)

This is the primary benchmark that I'm trying to optimize for with that: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=63735c6a61638d343688d98380f717b7. But also, http-0.2.0.

[panstromek]

Thanks, makes sense, I'll keep experimenting here then.

I'm separately trying to refactor the live loan computation to be loans-per-point, rather than points-per-loan - which I think will conflict more.

Oh, then you might probably want to look at #161850. That's just a data structure swap, though, doesn't really change the computation.

(Though, if you're in the area and want to work on that, then that's okay.)

I still have only surface level understanding of the algorithm, so I don't want to lick that cookie atm :D.

[lqd]

What's the thinking behind this? I think there aren't that many locals in our benchmarks IIRC(?), though you may want to check the following crates instead (no idea about the relevant/boring distinction for these, for this PR I only have the max locals seen in some of their functions):

• poestat_static-0.1.0: 3688118
• ostn02_phf-0.1.13: 1301165
• lazy_static-1.5.0: 846498

In liveness we separate the two and iterate through all these anyways, i.e. never needing to check if a given local is present in either set, and only do that boring vs relevant check in diagnostics for actual borrowck errors: we won't see such cases in benchmarks or crates.io.

[panstromek]

What's the thinking behind this?

Mostly just an experiment. I want to see whether I can slowly morph some of the code that uses these to use set operations on those bitsets instead of iterating on each local separately.

We can also avoid some memcopies and save memory, but that's not very impactful atm (sadly MaxRSS peak is probably not here, so it's difficult to tell if this helps memory usage even a little bit in some way).

lazy_static-1.5.0: 846498

wow, how is that even possible? That crate contains almost nothing.

[lqd]

Most likely some error in accounting due to it relying on macros sorry, something like its users' functions only showing up as spans from that crate instead of their own; I'll have to see which dependent crates are actually causing these statics with big locals.

[rust-bors]

☔ The latest upstream changes (presumably #162148) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.