ParamEnv::def_id is expensive

[nnethercote]

ParamEnv::def_id is only used for Chalk, which currently isn't on by default. I tried removing that field, which (among other things) reduces the size of the hot PredicateObligation type from 40 bytes to 32 bytes. Here are some local perf results on Full builds.

ctfe-stress-4-opt
        avg: -3.1%?     min: -3.1%?     max: -3.1%?
unicode_normalization-check
        avg: 3.0%       min: 3.0%       max: 3.0%
ctfe-stress-4-check
        avg: -2.6%?     min: -2.6%?     max: -2.6%?
ctfe-stress-4-debug
        avg: -2.5%?     min: -2.5%?     max: -2.5%?
unicode_normalization-debug
        avg: 2.1%       min: 2.1%       max: 2.1%
piston-image-debug
        avg: -1.6%      min: -1.6%      max: -1.6%
regression-31157-debug
        avg: -1.6%      min: -1.6%      max: -1.6%
encoding-debug
        avg: -1.6%      min: -1.6%      max: -1.6%
deeply-nested-debug
        avg: -1.5%      min: -1.5%      max: -1.5%
webrender-debug
        avg: -1.5%      min: -1.5%      max: -1.5%
cargo-debug
        avg: -1.5%      min: -1.5%      max: -1.5%
serde-check
        avg: -1.5%      min: -1.5%      max: -1.5%
tokio-webpush-simple-debug
        avg: -1.4%      min: -1.4%      max: -1.4%
unicode_normalization-opt
        avg: 1.4%       min: 1.4%       max: 1.4%
serde-debug
        avg: -1.4%      min: -1.4%      max: -1.4%
encoding-opt
        avg: -1.4%      min: -1.4%      max: -1.4%
webrender-wrench-debug
        avg: -1.4%      min: -1.4%      max: -1.4%
issue-46449-debug
        avg: -1.3%      min: -1.3%      max: -1.3%
syn-debug
        avg: -1.3%      min: -1.3%      max: -1.3%
cranelift-codegen-debug
        avg: -1.3%      min: -1.3%      max: -1.3%       
futures-debug
        avg: -1.3%      min: -1.3%      max: -1.3%
futures-check
        avg: -1.3%      min: -1.3%      max: -1.3%
serde-opt
        avg: -1.3%      min: -1.3%      max: -1.3%
clap-rs-debug
        avg: -1.3%      min: -1.3%      max: -1.3%
ripgrep-debug
        avg: -1.3%      min: -1.3%      max: -1.3%
deeply-nested-check
        avg: -1.2%      min: -1.2%      max: -1.2%
hyper-2-debug
        avg: -1.1%      min: -1.1%      max: -1.1%
regex-debug
        avg: -1.1%      min: -1.1%      max: -1.1%
deep-vector-debug
        avg: -1.1%      min: -1.1%      max: -1.1%

(The unicode_normalization regression is weird, because this change results in the code clearly doing less work overall. Maybe it's a quirk of inlining or codegen, the kind of thing that may well disappear in the face of other minor changes.)

Is this field really needed?

cc @rust-lang/wg-compiler-performance

[nikomatsakis]

We could probably remove it. Have to think a bit about it. It shouldn't eventually be needed for chalk, but it permits us to have a crutch for now, since chalk constructs its parameter environments differently (and hence the def_id allows us to recalculate those environments. cc @jackh726 I don't remember if we had any better ideas for how to handle parameter environments, I remember discussing them some when we were working on the chalk integration.

[jackh726]

I think this is that relevant discussion.

We didn't really go into it, since I think it was somewhat unrelated. Unfortunately, I haven't looked into alternatives here and don't even really know much about the implementation history for this, since this particular bit of code comes from the original integration.

I imagine we could either try to "elaborate" the environment when we actually create a ParamEnv (under -Z chalk), which would require adding a TypeFromEnv predicate; or we could somehow keep track of the ParamEnv environment def_id while trait solving, though I don't really know the rustc code well enough to comment on how difficult/plausible that would be. I think the former would be easier and probably what we want in the long run (in terms of Chalk integration) anyways.

As a side note, looking through uses of ParamEnv::new, this is the only instance of a ParamEnv being constructed with a DefId (that isn't ParamEnv being contstructed based on an existing ParamEnv), so it would be really easy to just insert the TypeFromEnv elaboration here.

Somewhat related, but I think some of the cases where a ParamEnv is constructed without a DefId are wrong or this logic is wrong.

[nikomatsakis]

@jackh726 I think it's the case that chalk's parameter environments just have strictly more things in them, is that right? But are all of those predicates things we can represent (and faithfully interconvert) with rustc's ty::Predicate?

[jackh726]

See ChalkEnvironmentClause.

I think it's the case that chalk's parameter environments just have strictly more things in them, is that right?

Yes

But are all of those predicates things we can represent (and faithfully interconvert) with rustc's ty::Predicate?

No, we need a TypeFromEnv

[nikomatsakis]

Yeah, I see. So we could extend the set of rustc predicates and then we could just have larger parameter environments when using chalk?

[jackh726]

Yes, that could work.

[nnethercote]

Is there a clear path forward for this? I'm happy to help but I know nothing about Chalk...

[jackh726]

@nnethercote yes

So, the first thing to do would be to add a TypeFromEnv(Ty<'tcx>) variant to PredicateAtom, which is only used for Chalk. It would be lowered to Chalk's FromEnv(FromEnv::Type), like here.

Then, you would essentially have to add those predicates here.

From there, you should be able to remove the environment elaboration in chalk_fulfill. As cleanup, you could also remove ChalkEnvironmentAndGoal and just use the PredicateObligation in chalk_fulfill.

Let me know if that makes sense! I'll be around on Zulip if you have questions or need to discuss.

[vandenheuvel]

@rustbot claim

[vandenheuvel]

@nnethercote or @jonas-schievink could this be closed?