Make erase_regions_ty no_hash and remove support for anon queries

[Zoxc]

r? @michaelwoerister

[Zoxc]

@bors try

[bors]

⌛ Trying commit 423e679 with merge 599cd5e...

[bors]

☀️ Try build successful - checks-travis
Build commit: 599cd5e

[Zoxc]

@rust-timer build 599cd5e

[rust-timer]

Success: Queued 599cd5e with parent d70c5a9, comparison URL.

[Dylan-DPC-zz]

ping from triage @michaelwoerister waiting for your review on this

[rust-timer]

Finished benchmarking try commit 599cd5e

[michaelwoerister]

Trait selection still needs anonymous dep-nodes. I'd love to get rid of anonymous queries but it might be blocked on Chalk integration (or at least doing something similar). cc @rust-lang/wg-traits

I'd be interested in the results of making erase_regions_ty no_hash but I suspect that it leads to a lot of invalidation downstream.

[Zoxc]

Trait selection still needs anonymous dep-nodes.

Yeah that is why I kept that and explicitly said anonymous queries. Is there any particular reason we hash the dependencies of anon nodes instead of just storing them directly? I may be looking to change that, though I'd prefer no anon nodes.

Would it be possible for trait selection to use a regular dep node by hashing the input or something like that?

[Zoxc]

I'd be interested in the results of making erase_regions_ty no_hash but I suspect that it leads to a lot of invalidation downstream.

Given that erase_regions_ty is supposed to have no dependencies, it will always be marked green, so there's no downside from no_hash.

[michaelwoerister]

Yeah that is why I kept that and explicitly said anonymous queries.

It would be really helpful if you provided descriptions for your PRs. Some background information, the motivation for doing the change, things of note that are easily overlooked, potentially contentious consequences of the change, etc.

Given that erase_regions_ty is supposed to have no dependencies, it will always be marked green, so there's no downside from no_hash.

Hm, that's a rather subtle side-effect of how try-mark-green works but I guess it's even correct :)

I'm not sure what to make of the performance results. Except for ctfe-stress-2 instruction counts seem to regress slightly.

Is there any particular reason we hash the dependencies of anon nodes instead of just storing them directly?

What do you mean exactly?

Would it be possible for trait selection to use a regular dep node by hashing the input or something like that?

I don't remember the exact reasons, @nikomatsakis took care of trait selection dep-tracking. Something in the way the caching for trait selection works made anon nodes necessary.

[Zoxc]

I forgot to include the fix from the other PR. Let's see if the servo regression goes away with that.

@bors try

[bors]

⌛ Trying commit cc7b4c2 with merge 270dea6...

[Zoxc]

Is there any particular reason we hash the dependencies of anon nodes instead of just storing them directly?

I guess we already store the dependencies already. Maybe I was thinking of some scheme were we generate an unique dep node based on the dependencies ([DepNodeIndex]) which could use FxHash and a hash map, but I guess that wouldn't really change much.

[bors]

⌛ Trying commit cc7b4c2 with merge bf680dd88be9b5d89e7a6d8969d053981fa9196b...

[bors]

💔 Test failed - checks-travis

[Zoxc]

@bors try

[Zoxc]

@bors retry

[bors]

⌛ Trying commit cc7b4c2 with merge 7b167f5...

[bors]

☀️ Try build successful - checks-travis
Build commit: 7b167f5

[Zoxc]

@rust-timer build 7b167f5

[rust-timer]

Success: Queued 7b167f5 with parent 07133ac, comparison URL.

[rust-timer]

Finished benchmarking try commit 7b167f5

[michaelwoerister]

Performance numbers look like a slight regression. Since we are in no rush to remove anonymous queries, I suggest we wait for a solution along the lines of #59505.