Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove allow(rustc::potential_query_instability) in rustc_trait_selection #104212

Closed
CastilloDel opened this issue Nov 9, 2022 · 3 comments
Closed

Comments

@CastilloDel
Copy link
Contributor

Related to #84447, #103723 and indexmap-rs/indexmap#242

rustc_trait_selection still has some potential query instabilities, however, trying to remove them caused regressions. This issue tracks this problem after #103723 was merged. The idea is to wait until the drain_filter method is eventually added to IndexMap which would make the change easier and hopefully solve the regression.

@ndrewxie
Copy link
Contributor

ndrewxie commented Apr 7, 2023

Correct me if I'm wrong, but the only place I see drain_filter being used on the map field of ProvisionalEvaluationCache is the following piece of code:

for (fresh_trait_pred, eval) in
    self.map.borrow_mut().drain_filter(|_k, eval| eval.from_dfn >= dfn)
{
    debug!(?fresh_trait_pred, ?eval, "on_completion");
}

This looks like it could just be replaced with a .retain without any performance penalty, right? Because debug doesn't need to own the value

I'll try it out later and see if it works

bors added a commit to rust-lang-ci/rust that referenced this issue Apr 8, 2023
Switched provisional evaluation cache map to FxIndexMap, and replaced map.drain_filter with map.retain

Switching ProvisionalEvaluationCache's map field from FxHashMap to FxIndexMap was previously blocked because doing so caused performance regressions that could be mitigated by the stabilization of drain_filter for FxIndexMap (rust-lang#104212). However, the only use of drain_filter can be replaced with a retain, so I made the modification and put in a PR to see if this causes a performance regression as well.

This PR is part of a broader effort (rust-lang#84447) of removing iteration through FxHashMaps, as the iteration order is unstable and can cause issues in incremental compilation.
@ndrewxie
Copy link
Contributor

ndrewxie commented Apr 8, 2023

Alright this appears to be resolved - can someone please close this issue? Thanks! (I messed up the wording of the PR and didn't link the issues properly)

@jyn514
Copy link
Member

jyn514 commented Apr 8, 2023

Sure thing, glad you got this working :)

@jyn514 jyn514 closed this as completed Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants