Add a per-tree cache into the obligation forest

[nikomatsakis]

Have the ObligationForest keep some per-tree state (or type T) and have it give a mutable reference for use when processing obligations. In this case, it will be a hashmap. This obviously affects the work that @soltanmm has been doing on snapshotting. I partly want to toss this out there for discussion.

Fixes #31157. (The test in question goes to approx. 30s instead of 5 minutes for me.)
cc #30977.
cc @aturon @arielb1 @soltanmm

r? @aturon who reviewed original ObligationForest

[nikomatsakis]

cc @jroesch

[soltanmm]

@nikomatsakis
The following comment assumes that this PR is going to go in and that the snapshotting PR (#31175) will need to handle it somehow:

I believe you raised a point earlier that you were considering fully persistent hash maps in the per-tree state as a means to simplify the interface between the ObligationForest and other code (I guess by just making the bounds on the per-tree state include Clone), but do you feel particularly strongly about that? I mean as opposed to having bounds on the per-tree state, e.g. Snapshottable (wc?) or perhaps something that can push undo actions into the forest.

It seems like using a HashMap-like-thing will result in similar added complexity on the part of the (aspiring-to-be-performant) client whether it ends up using a fully persistent structure or one that provides snapshotting in some form. Although, going down the Clone route does make it plausible to postpone implementing a fully persistent hash map (by just taking the cloning hit on every snapshot), so, I'unno.

[nikomatsakis]

@soltanmm

I believe you raised a point earlier that you were considering fully persistent hash maps in the per-tree state as a means to simplify the interface between the ObligationForest and other code (I guess by just making the bounds on the per-tree state include Clone), but do you feel particularly strongly about that? I mean as opposed to having bounds on the per-tree state, e.g. Snapshottable (wc?) or perhaps something that can push undo actions into the forest.

Not very strongly. Something like snapshottable might be better, though we'd have to think about how it works (in particular, how the actions are accumulated -- maybe you could create a "handle" to the main value that also collects the undo actions locally or something). The other option would be just hard-coding the fact that the per-tree state is a map, and parameterizing ObligationForest with and K and V value for this map. This would allow the undo log to be tightly tied to the ObligationForest data structure itself.

[aturon]

Sorry for the delay reviewing. This looks basically fine to me -- at little hacky, but well-documented as such :)

@bors: r+

[bors]

📌 Commit 35d6efb has been approved by aturon

[bors]

⌛️ Testing commit 35d6efb with merge 6dc112d...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

[nikomatsakis]

Nominating for beta since this regression was pretty significant.

[nikomatsakis]

Approving for backport, although another option might to revert the change that introduced the ObligationForest. This would also fix the regression in #31299, at least for a little while. ;)

[White-Oak]

@nikomatsakis beta is going to become stable in 10 days, will this one this be backported until then?

[nikomatsakis]

I opted to revert the change on beta, because this did not backport entirely cleanly: #31851