Type subsumption cache: handle unsolved type vars

[majocha]

Description

Fixes #19037

Added repro test case.

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/11.0.0.md

[majocha]

Yet another bug. This needs a careful look.

I need to run some benchmarks again, too and compare standalone builds with the original Vlad's implementation.

[T-Gro]

@majocha:
If we separated the solved and unsolved form of a typar, why wouldn't we want to cache (type subsumption especially) for both?
.e.g. constraints might be already there and the cache would hold info on type subsumption of generic typars.

It would equally help if two typars are both unsolved, but strip down to the same form.

Wasn't the mistake more the fact that solved/unsolved form of the same TType_var had always the same key (based on Stamp) ?

This is definitely a safe fix, but I wonder if it doesn't too much limit the potential of the cache for typars.

[majocha]

Wasn't the mistake more the fact that solved/unsolved form of the same TType_var had always the same key (based on Stamp) ?

Yes, that makes sense. It makes me wonder if we could also handle unsolved nullness better.

[T-Gro]

Nullness is not stamped, so in the case of unresolved/not yet resolved/ nullness, there is no identifier to hold onto.
The identity of the NullnessVar (possibly the leaf one if multiple are sequentially linked) object itself (via https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimehelpers.gethashcode?view=net-9.0 )

[majocha]

This fix is accidental, the real problem is here:

fsharp/src/Compiler/Utilities/TypeHashing.fs

Line 480 in 9670f60

Extras.WeakMap.getOrCreate (fun ty -> accumulateTType ty |> toTypeStructure)

This was intended to speed up things by weakly attaching the computed TypeStructures to their respective TTypes. We cannot do it for not fully solved types, they still mutate and things get outdated.

[majocha]

Wasn't the mistake more the fact that solved/unsolved form of the same TType_var had always the same key (based on Stamp) ?

Yes, in fact in case of typars we don't want the stamp as identity at all. We should use the structure of the solution (TType) if available or a common token for unsolved.

[majocha]

I'm testing this in the IDE with the notorious OpenTK 5.0. It improved things significantly, there are way less cache entries now but much more hits, resulting in 99% ratio with little memory use and no constant churn during edits:

|             Cache name              | hit-ratio | adds | updates |  hits   | misses | 
|-------------------------------------|-----------|------|---------|---------|--------|
| typeSubsumptionCache                |    99.94% | 2752 |       0 | 4618992 |   2752 |

[T-Gro]

Now that the stamp is gone, I feel we are missing something to differentiate constraints.

Most likely types where constraints matter will be unsolved, so this will bypass the cache (via shouldCache=false) anyway. Right now cannot come up with a specific example of solved types where also constraints would matter for subsumption.

[majocha]

shouldCache here only affects the memoization of TType -> TypeStructure, it will not prevent the caching of checkSubsumes result for unsolved.

Yeah this is missing constraints. I feel encoding them could cause this to explode in size.
What I completely missed also is the notion of rigidity. As I understand not all variables end up solved.

[T-Gro]

In general, the input going into the cache is already stripped, right?

i.e. we should not be getting long chains of solution pointers for something which is solved.

[majocha]

Yeah since this is operating solely on stripped types, if we encounter a type var, it should never be a solved one. In theory.

[majocha]

The huge mistake that causes a lot of inefficiency was emitting typar stamps as part of the cache key. This causes a lot of equivalent but really unusable keys polluting the cache. The increased churn is especially heavy on the IDE. I cannot see a solution to this, that would guarantee soundness.

[T-Gro]

The huge mistake that causes a lot of inefficiency was emitting typar stamps as part of the cache key. This causes a lot of equivalent but really unusable keys polluting the cache. The increased churn is especially heavy on the IDE. I cannot see a solution to this, that would guarantee soundness.

What about skipping cache for them?
For unsolved typars, the typefeasiblysubsubsumestypes is a quick decision in case of TType_var anyway.

Of course it somewhat limits the genericity of the cache, and is special-tailored to the place of application.

The typesubsumption was really needed for concrete types, with super types and interface hierarchies.
Maybe if Ttype_var is not part of it (after stripping), we can have both correctness and good cache contents?

I was thinking of content-based hashing, but you would need to hash all type constraints content-wise as they are mutable as well. Maybe the inherent mutation for solutions and constraints of a type var are good enough reasons to treat them differently?
(mutation is not well suited for cache keys)