Remove allow(rustc::potential_query_instability) in rustc_const_eval #102674

CastilloDel · 2022-10-04T19:21:38Z

The use of FxHashMap has been replaced with FxIndexMap.

Related to #84447

rustbot · 2022-10-04T19:21:41Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2022-10-04T19:21:41Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

Please see the contribution instructions for more information.

oli-obk · 2022-10-05T07:07:17Z

@bors try @rust-timer queue

rust-timer · 2022-10-05T07:07:18Z

Awaiting bors try build completion.

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

bors · 2022-10-05T07:07:25Z

⌛ Trying commit 5791045a2f49613b4135a01099b7b9b465a72d4e with merge 7c0a7d4c564592173aaa1dd1dca36b9126a0f51c...

bors · 2022-10-05T08:29:36Z

☀️ Try build successful - checks-actions
Build commit: 7c0a7d4c564592173aaa1dd1dca36b9126a0f51c (7c0a7d4c564592173aaa1dd1dca36b9126a0f51c)

rust-timer · 2022-10-05T08:29:39Z

Queued 7c0a7d4c564592173aaa1dd1dca36b9126a0f51c with parent d8613f7, future comparison URL.

RalfJung · 2022-10-05T09:17:07Z

I guess we need an IndexMap because we iterate over this for interning?

rust-timer · 2022-10-05T09:51:14Z

Finished benchmarking commit (7c0a7d4c564592173aaa1dd1dca36b9126a0f51c): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.3%, 0.6%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-4.4%, -1.1%]	9
All ❌✅ (primary)	-	-	0

Cycles

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

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

CastilloDel · 2022-10-05T10:06:43Z

I guess we need an IndexMap because we iterate over this for interning?

Yes, from what I understand it is to make compilations reproducible as @/Aaron1011 explained:

The iteration order of a HashMap depends on the Hash value of the keys. In rustc, the Hash value of a key can change between compilation sessions, even if the HashStable value remains the same (e.g. DefId, Symbol, etc.). This means that iterating over a HashMap can cause a query to produce different results given the same (as determined by HashStable) input. This can be quite subtle, as demonstrated by #82272 (comment)

If we are sure that iterating over the FxHashMap in this case won't cause reproducibility issues, we could add comments explaining why that's the case. The problem is, I don't think that is the case here (?). I'm not sure, as I'm new to this.

I also don't know if that regression is acceptable or not. 0.5% seems to be significant.

oli-obk · 2022-10-05T10:52:13Z

If we are sure that iterating over the FxHashMap in this case won't cause reproducibility issues, we could add comments explaining why that's the case.

It doesn't cause problems here, because all we do is insert Allocations for already existing AllocIds into the TyCtxt. There is nothing that can possibly go wrong here from an ordering perspective. No matter the order, the end result is exactly the same.

CastilloDel · 2022-10-05T11:51:44Z

If we are sure that iterating over the FxHashMap in this case won't cause reproducibility issues, we could add comments explaining why that's the case.

It doesn't cause problems here, because all we do is insert Allocations for already existing AllocIds into the TyCtxt. There is nothing that can possibly go wrong here from an ordering perspective. No matter the order, the end result is exactly the same.

Then I suppose it's better now.

Is the explanation enough? I would like to do it better, but I'm afraid I don't know enough about rustc_const_eval to do it.

RalfJung · 2022-10-05T12:13:41Z

It doesn't cause problems here, because all we do is insert Allocations for already existing AllocIds into the TyCtxt. There is nothing that can possibly go wrong here from an ordering perspective. No matter the order, the end result is exactly the same.

That's pretty subtle though. We only have regression in secondary benchmarks (stress tests), nothing primary, so this might be fine in exchange for definitely not having subtle query bugs.

Up to you though, I don't understand the query stuff that well anyway.^^

CastilloDel · 2022-10-13T16:35:56Z

I's been a week, so I will try to sum everything up.

There are two options:

Just add comments. This option is what is currently on the PR, because @oli-obk said it was okay. However, I don't understand this enough to make better comments than the current ones, and I believe they should be better.
Substitute FxHashMap with FxIndexMap. This causes a 0.5% regression, which @RalfJung deemed okayish.

RalfJung · 2022-10-16T07:45:23Z

An 0.5% regression in secondary benchmarks only, to be specific: tuple-stress and coercions.

oli-obk · 2022-10-18T12:40:53Z

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -131,6 +131,9 @@ impl<K: Hash + Eq, V> interpret::AllocMap<K, V> for FxHashMap<K, V> {

    #[inline(always)]
    fn filter_map_collect<T>(&self, mut f: impl FnMut(&K, &V) -> Option<T>) -> Vec<T> {


This site depends on the callers, but the only caller is the leak checker. So we could either make the function name end with like _only_use_in_miri or just fix it and let miri take the perf hit

I think the perf hit should be tiny for the usual case where programs have freed most of their memory when they terminate.

But there's another perf hit somewhere, right? After all we are seeing a regression in the benchmarks.

Yes, in the interner, that one is unavoidable.

oli-obk · 2022-10-18T12:44:35Z

That's pretty subtle though. We only have regression in secondary benchmarks (stress tests), nothing primary, so this might be fine in exchange for definitely not having subtle query bugs.

Agreed, I'd still want to figure out a way to get the best of both worlds: guaranteed correctness and performance. But that doesn't have to happen here.

So let's take the perf hit, basically changing everything back to your original PR. Sorry about the back and forth

The use of FxHashMap has been replaced with FxIndexMap. For more information see rust-lang#84447

CastilloDel · 2022-10-18T15:52:26Z

Sorry about the back and forth

Don't worry about it! I already brought back the initial changes and also rebased the fork.

oli-obk · 2022-10-18T16:47:00Z

@bors r+ rollup=never

bors · 2022-10-18T16:47:02Z

📌 Commit c3a1ca6 has been approved by oli-obk

It is now in the queue for this repository.

CastilloDel · 2022-10-28T10:14:16Z

Sorry, but I was working on #103218 and I noticed I made a mistake in this PR. The reproducibility guarantees that IndexMap offers only work as long as .remove isn't used, which isn't true for this PR. For it to work, the remove should be changed to shift_remove, which could potentially impact the performance.

oli-obk · 2022-10-28T10:18:54Z

@bors r-

oli-obk · 2022-10-28T10:22:15Z

Oof, that's subtle, but makes sense. I'm really not sure on the best way to fix this beyond moving to a more salsa like scheme.

RalfJung · 2022-10-28T10:36:52Z

So the lint is not able to capture that? This needs careful documentation...

oli-obk · 2022-10-28T10:40:02Z

So the lint is not able to capture that? This needs careful documentation...

yea, I think we should rename remove to unstable_swap_remove and rename shift_remove to remove.

CastilloDel · 2022-10-28T12:03:13Z

Okay, it seems I was wrong. Calling remove disrupts the order (it isn't the insertion order anymore), but it is still reproducible. This can be seen in this test

Sorry for the back and forth 😥 I should have checked this before. This PR is okay as it is and I'll remove the shift_remove in #103218, as it isn't needed for reproducibility purposes.

oli-obk · 2022-10-28T12:11:21Z

Okay, it seems I was wrong. Calling remove disrupts the order (it isn't the insertion order anymore), but it is still reproducible. This can be seen in this test

Hmm... that's only true if the order of items we remove is reliable. I guess that's the case here, but very very subtle

CastilloDel · 2022-10-28T12:21:31Z

Hmm... that's only true if the order of items we remove is reliable. I guess that's the case here, but very very subtle

Yes, but in that case the reproducibility problem would come from another source, which is the one that should get fixed. The compilation sessions should be totally reproducible, which is why we are doing this. I'm inclined to think that probably it won't be an issue, but I could be wrong. I guess it could produce confusion on the origin of a bug, though.

Should I change #103218 ? Maybe that merge should be stopped, until we are sure on how to proceed.

oli-obk · 2022-10-28T12:24:27Z

Should I change #103218 ? Maybe that merge should be stopped, until we are sure on how to proceed.

No, your reasoning seems correct to me.

@bors r+

bors · 2022-10-28T12:24:28Z

📌 Commit c3a1ca6 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-10-28T12:52:20Z

⌛ Testing commit c3a1ca6 with merge 5237c4d...

bors · 2022-10-28T16:06:58Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 5237c4d to master...

rust-timer · 2022-10-28T17:25:42Z

Finished benchmarking commit (5237c4d): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	0.6%	[0.3%, 0.8%]	18
Improvements ✅ (primary)	-0.6%	[-1.4%, -0.2%]	5
Improvements ✅ (secondary)	-2.7%	[-3.9%, -0.3%]	8
All ❌✅ (primary)	-0.5%	[-1.4%, 0.4%]	6

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-10-30T23:09:07Z

The keccak, wg-grammar, and cranelift-codegen results here are noise. The other changes (mostly tuple-stress, coercions, ucd) are real, but they were deemed acceptable above.

@rustbot label: +perf-regression-triaged

Remove allow(rustc::potential_query_instability) in rustc_const_eval The use of FxHashMap has been replaced with FxIndexMap. Related to rust-lang#84447

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 4, 2022

rust-highfive assigned davidtwco Oct 4, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 5, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 5, 2022

CastilloDel force-pushed the master branch from 5791045 to 9098161 Compare October 5, 2022 11:48

oli-obk reviewed Oct 18, 2022

View reviewed changes

Remove allow(rustc::potential_query_instability) in rustc_const_eval

c3a1ca6

The use of FxHashMap has been replaced with FxIndexMap. For more information see rust-lang#84447

CastilloDel force-pushed the master branch from 9098161 to c3a1ca6 Compare October 18, 2022 15:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 28, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 28, 2022

This was referenced Oct 28, 2022

Change remove to unstable_remove indexmap-rs/indexmap#241

Closed

Remove #![allow(rustc::potential_query_instability)] from rustc_infer #103218

Merged

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 28, 2022

bors merged commit 5237c4d into rust-lang:master Oct 28, 2022

rustbot added this to the 1.66.0 milestone Oct 28, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 30, 2022

		@@ -131,6 +131,9 @@ impl<K: Hash + Eq, V> interpret::AllocMap<K, V> for FxHashMap<K, V> {

		#[inline(always)]
		fn filter_map_collect<T>(&self, mut f: impl FnMut(&K, &V) -> Option<T>) -> Vec<T> {

Remove allow(rustc::potential_query_instability) in rustc_const_eval #102674

Remove allow(rustc::potential_query_instability) in rustc_const_eval #102674

Conversation

CastilloDel commented Oct 4, 2022

rustbot commented Oct 4, 2022

rust-highfive commented Oct 4, 2022

oli-obk commented Oct 5, 2022

rust-timer commented Oct 5, 2022

bors commented Oct 5, 2022

bors commented Oct 5, 2022

rust-timer commented Oct 5, 2022

RalfJung commented Oct 5, 2022

rust-timer commented Oct 5, 2022

Overall result: ❌ regressions - ACTION NEEDED

Footnotes

CastilloDel commented Oct 5, 2022

oli-obk commented Oct 5, 2022

CastilloDel commented Oct 5, 2022

RalfJung commented Oct 5, 2022

CastilloDel commented Oct 13, 2022

RalfJung commented Oct 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Oct 18, 2022

CastilloDel commented Oct 18, 2022

oli-obk commented Oct 18, 2022

bors commented Oct 18, 2022

CastilloDel commented Oct 28, 2022

oli-obk commented Oct 28, 2022

oli-obk commented Oct 28, 2022

RalfJung commented Oct 28, 2022 via email

oli-obk commented Oct 28, 2022 • edited Loading

CastilloDel commented Oct 28, 2022

oli-obk commented Oct 28, 2022

CastilloDel commented Oct 28, 2022 • edited Loading

oli-obk commented Oct 28, 2022

bors commented Oct 28, 2022

bors commented Oct 28, 2022

bors commented Oct 28, 2022

rust-timer commented Oct 28, 2022

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Footnotes

nnethercote commented Oct 30, 2022

oli-obk commented Oct 28, 2022 •

edited

Loading

CastilloDel commented Oct 28, 2022 •

edited

Loading