Disable hashing on effective_visibilities

[blyxyas]

Make effective_visibilities no_hash. We were wasting too much time on hashing the FxIndexMap (15 million instructions on tokio just for the hashing) so it was not worth it.

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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

[jieyouxu]

@bors2 try @rust-timer queue

[rust-bors]

⌛ Trying commit 64189bf with merge afc29b9…

To cancel the try build, run the command @bors try cancel.

[nnethercote]

Hmm, I am not familiar with the query modifiers like no_hash and eval_always and the documentation isn't very clear, so I'm not comfortable reviewing this. @blyxyas, can you explain how/why this is a valid and good change? Thanks!

[rust-bors]

☀️ Try build successful (CI)
Build commit: afc29b9 (afc29b9bbebfe04fc6635a69253e150adb83e511, parent: 0060d5a2a8a86a31f6299311fe64b1d755a91c4f)

[rust-timer]

Finished benchmarking commit (afc29b9): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@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)	3.0%	[0.2%, 11.0%]	83
Regressions ❌ (secondary)	0.9%	[0.2%, 3.7%]	18
Improvements ✅ (primary)	-2.2%	[-11.0%, -0.3%]	45
Improvements ✅ (secondary)	-1.0%	[-2.5%, -0.1%]	36
All ❌✅ (primary)	1.2%	[-11.0%, 11.0%]	128

Max RSS (memory usage)

Results (primary 1.9%, secondary -0.1%)

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

	mean	range	count
Regressions ❌ (primary)	1.9%	[0.7%, 4.2%]	18
Regressions ❌ (secondary)	2.1%	[1.9%, 2.4%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.4%, -2.3%]	2
All ❌✅ (primary)	1.9%	[0.7%, 4.2%]	18

Cycles

Results (primary 3.4%, secondary -2.0%)

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

	mean	range	count
Regressions ❌ (primary)	6.0%	[2.6%, 10.2%]	40
Regressions ❌ (secondary)	3.5%	[2.7%, 4.0%]	3
Improvements ✅ (primary)	-4.5%	[-7.9%, -2.1%]	13
Improvements ✅ (secondary)	-4.4%	[-6.2%, -2.0%]	7
All ❌✅ (primary)	3.4%	[-7.9%, 10.2%]	53

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 466.337s -> 468.665s (0.50%)
Artifact size: 376.97 MiB -> 376.94 MiB (-0.01%)

[nnethercote]

Those perf results are not so good, unfortunately :(

[blyxyas]

:/ Curious results, but not what I'm looking for.

can you explain how/why this is a valid and good change? Thanks!

The principle for this change was that if the effective_visibilities query was very prone to change, and produced a very complex value (an FxIndexMap), so hashing it didn't make much sense.

Also, while inspecting some Valgrind output, we were wasting a significant amount of time on hashing EffectiveVisibiltiies types. Kinda weird that in my cachegrind output it looked like an improvement.

NOTE: Thinking about it, I just remembered that I have the bad habit of only benchmarking the Full scenario and I should change that.

[cjgillot]

Also, while inspecting some Valgrind output, we were wasting a significant amount of time on hashing EffectiveVisibiltiies types. Kinda weird that in my cachegrind output it looked like an improvement.

This appears in the perf results. The -11% improvements show this.

My understanding of the regressions:
Marking the query no_hash means it only has information about dependencies to know whether the result has changed. And effective_visibilities tends to look at the whole crate's contents, so will very probably be marked red, even if the results are not changed. So dependent queries need to be recomputed, only to discover their results are unchanged.

A possible way forward would be to hide the large effective_visibilities array behind a targeted query effective_visibility(LocalDefId) -> EffectiveVisibility. This would have a query overhead, but may avoid recomputing of some dependents.