Experiment: Plug caches in

[majocha]

Description

Just connect #18190 to see what the performance is in the IDE.

[github-actions]

❗ Release notes required

@majocha,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.300.md	No release notes found or release notes format is not correct
vsintegration/src	docs/release-notes/.VisualStudio/17.14.md	No release notes found or release notes format is not correct

[majocha]

As usual I installed this in Visual Studio. Things seem to work normally. Performance feels good.

[vzarytovskii]

I suggest getting stats for project during working with it in ide and adjusting cache size to avoid resizes.

[vzarytovskii]

Also, perf benefits won't show on normal solution, you can take the original openmp one from my original fix

[majocha]

Original fix for reference: #17668, because every time I'm looking for it I can't find it 😄
I'll try to record some stats with OTEL later.

[T-Gro]

I wonder if there would be any drawbacks in implementing that ToArray special handling (via a typecheck) in the Seq. module directly.
This is an unpleasant bug to deal with and I cannot thing of regressions when moving from old behavior to the one suggested here.
(big drawback is the added cost of type test for every single user, I do understand that :( )

[vzarytovskii]

I think it can be implemented and documented as "technically breaking, but correct change". I didn't bother with it, and just did the implementation I required in the cache itself.

Can we do it without type test via the inline compiler-only feature (i.e. type testing on type level and choosing correct implementation)?

module Seq =
    let sortBy<'T> projection source =
        checkNonNull "source" source

        mkDelayedSeq (fun () ->
            let array = source |> toArray
            Array.stableSortInPlaceBy projection array
            array :> seq<_>)

       when 'T : ConcurrentDictionary<_> =
          checkNonNull "source" source
          mkDelayedSeq (fun () ->
             let array = source.ToArray()
             Array.sortInPlaceBy projection array
             array :> seq<_>)

This way, there's no runtime type test drawback.

[majocha]

I'm not sure if this is sensible at all in light of #17668 (comment)
Well, there are no shortcuts to understanding what you do before you do it :) I need to grasp this fully first.

[vzarytovskii]

I'm not sure if this is sensible at all in light of #17668 (comment)

Well, there are no shortcuts to understanding what you do before you do it :) I need to grasp this fully first.

With the cache, those comments are not really relevant anymore.

I've made cache keys rely on stamps (and more). Plus with the cache instead of CD, we now have eviction and in the worst case scenario sizes will need to be adjusted.

[majocha]

I've made cache keys rely on stamps (and more). Plus with the cache instead of CD, we now have eviction and in the worst case scenario sizes will need to be adjusted.

Ah, now I see, stampEquals. Awesome, thanks!

[majocha]

So, disregarding hit rate for now, I did count just unique keys that get cached.
Just browsing and navigating the FSharp.sln without any edits can produce hundreds of thousands of keys.

opening the handful of .fs files in OpenTK 5 branch, the count explodes to 200k immediately. Edits in some of its most affected files cause a lot of churn, too, but it is usable.

To see in action, start VisualFSharp debug build, total count will show up in Output window.

[majocha]

To reiterate, steps to test this with Visual Studio:

1. Clone OpenTK and checkout opentk5.0 branch

git clone https://github.com/opentk/opentk.git
cd opentk
git checkout opentk5.0

2. open OpenTK.sln, open Matrix4Tests.fs
3. observe time to typecheck / semantic colorization

on main it takes forever, on this PR branch it is few seconds.

[majocha]

Some ideas:

• provideIDictionary<, >, vary implementation based on CompilationMode. (ConcurrentDictionary when OneOff)
• provide an IDE option to override LanguageFeature restrictions in incremental use.

[vzarytovskii]

Question is what is the memory use increase in IDE when using cache?

[majocha]

That makes me think what unexpectedly mutates is the hashcode. It seems ConcurrentDictionary relies on it in TryRemove. That's why unevictable items accumulate during RL usage.

I tested this theory by making the GetHashCode() really unhelpful but stable:

override this.GetHashCode() : int = this.canCoerce.GetHashCode()

immediately the cache behaves perfectly:

Each row is circa one second. cache1 is current count, evicted, fails, hits, misses are just number of triggered events during the elapsed one second.

MaxCapacity is 10k. It can be seen the moment it's reached evictions start and do keep it without fails. Also a lot more cache hits, because now we can actually find items in the store.

I don't know how feasible this is but the fix I envision is to provide hashcode that is tied to the source text as much as possible, not to the mutable type resolutions. It means take ranges, display names, stuff like that, that doesn't mutate unless the user edits something.

update:

With this, TTypeCacheKey.Equals gets much more action, occasionally producing a false positive from here:

fsharp/src/Compiler/Utilities/TypeHashing.fs

Line 131 in 85ea990

| TType_app(tcref1, _, _), TType_app(tcref2, _, _) -> tcref1.Stamp.Equals(tcref2.Stamp)

[majocha]

The problem was in using the signature hash functions. I guess they will not produce a stable hash while the compilation is not done, constraints are still being solved and so on.
I wrote a stupid simple hashing function instead, just for the purpose of cache. It seems to be stable, but it's pure vibe-coding and it needs a double check.
I also found and fixed a small bug in stampEquals. It was giving false positives in some cases.

This seems to do the trick and things work ok now.

On the side of the cache, I added a simple eviction queue and an object pool to reuse nodes and keep allocations constant.
I should probably split this into two PRs.