refactor(borrowck): less cloning in compute_closure_requirements_modulo_opaques

[blueberry-dog]

Not too sure if this is the right solution but thought I would put up this PR as a starting point to iterate on. Looking at how the fields are used in compute_sccs_applying_placeholder_outlives_constraints and RegionInferenceContext, it seems like most can be used immutably but right now RegionInferenceContext expects to own these fields.

I tried to introduce a new lifetime to RegionInferenceContext so we can use references instead but it seems like a bit of a pain to update each of the struct definitions it's used in. Alternatively, we could pass the fields as parameters instead of keeping them in the structs which is easy for some fields like type_tests but would be more work for some of the others, though I think this might be a better solution since CollectRegionConstraintsResult can own the fields and then use a shared reference for the duration of compute_closure_requirements_modulo_opaques? For now, I've just used Rc on these fields but this is a bit unfortunate because it requires using Rc::get_mut in CollectRegionConstraintsResult methods and shifting the check of whether another reference exists to runtime. If we decide to go forward with using Rc, I'll at least add a few comments mentioning that we can't have an outstanding reference when calling certain methods.

This is the local performance difference between main and this PR:

Closes #3

[fee1-dead]

Overall, I think this is an okay start, but I would remove the usage of Rc::get_mut altogether. I would see if entire types can be wrapped into Rc instead of individual fields, and yeah, no Rc::get_mut because it is really footgunny and can definitely go wrong.

[fee1-dead]

apologies for the late review.

[fee1-dead]

I believe I understand the core of this issue now, thanks for making an effort to fix this. Here's what I think should happen: There should be a new struct that stores everything in MirTypeckRegionConstraints except outlives_constraints. We can extract them out because that doesn't need to be modified by compute_sccs_applying_placeholder_outlives_constraints (they just get passed forward). I'm pretty sure they can be references too.

The problem here is that now LoweredConstraints has duplicate fields: constraints.outlives_constraints and outlives_constraints. We want to avoid that.

[blueberry-dog]

apologies for the late review.

No worries! Thanks for the reviews, I appreciate them :)

I think doing the partial move is a good idea, I pushed a version of it illustrative purposes and it seems to get around the previous hacks I had for the Rc version which is nice.

I agree that passing by reference seems the best here but I can't seem to find a way to do it without adding a new lifetime parameter to RegionInferenceContext? For example we make the struct without outlives_constraints, it gets passed to LoweredConstraints and then that gets passed to RegionInferenceContext::new. Going to try it some more and see if I can come up with anything since it feels like passing by reference is the way to go.

[blueberry-dog]

Hey @fee1-dead, sorry for the ping. This study is wrapping up soon and unfortunately I can't see a better way to do this with references right now. I think we should either accept the partial move solution which is okay but maybe not ideal or I can just close this PR for now since it turned out to be a bit more complex than I originally thought.

[fee1-dead]

I will do an actual review later today, but my initial glance at it seems okay

[fee1-dead]

This is the right direction now I believe; Thanks for taking the time from all this back and forth! Now this is a strict improvement over the previous code.

[fee1-dead]

Also, really sorry that this PR got eclipsed by rust-lang#151688. I hope you can contribute to rustc itself sometime :)

[blueberry-dog]

This is the right direction now I believe; Thanks for taking the time from all this back and forth! Now this is a strict improvement over the previous code.

Thank you as well for your time!

Also, really sorry that this PR got eclipsed by rust-lang#151688. I hope you can contribute to rustc itself sometime :)

Oh cool, looks like at a glance that does it the proper way with references! I'll have to take a look and learn from it :)