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]

Store.Count metrics redirected to output in release mode:

It's a mess right now but usable for some experimentation.

[vzarytovskii]

Store.Count metrics redirected to output in release mode:

It's a mess right now but usable for some experimentation.

Not a call to action, but I'd be curious about the hit/miss ratio, and maybe overall mru/lru rate - I.e. how many keys are we consistently hitting 1) in the first typecheck and 2) in the rest of the tooling lifetime.

This should help figuring out the default sizes for different projects. Maybe even have some heuristic on how to grow/shrink the cache during the tooling use.

[majocha]

So I instrumented also eviction fails. Looks like there is a huge amount of them (per second and compared to successful evictions), both in blocking mode and background task. Hmmm.

My guess is a separate eviction candidates queue / linked list is a must.

[vzarytovskii]

So I instrumented also eviction fails. Looks like there is a huge amount of them (per second and compared to successful evictions), both in blocking mode and background task. Hmmm.

My guess is a separate eviction candidates queue / linked list is a must.

I was afraid that we might require something like that, just didn't want to overcomplicate it.

Evict failures aren't bad per se as long as we are maintaining cache properly, I.e. we evict the right amount, don't grow way beyond the capacity, and have the amount of items we expect at any given moment.

Update 1:

Actually, I don't fully understand what do those represent? Are they amount of times we tried to remove element from the cache, but it wasn't there by the time we tried? If so, during which time period?

Also, I don't think at this point I get when it can happen. If we evict synchronously on the same thread, or on separate thread (also synchronously), how they can already be evicted?

[majocha]

This is just failure count per second. I tried making everything synchronous and it's still there. I suspect equality / hashcode is not stable 😐

[majocha]

The thing it boils down to is we put something in the CD and later it can't be found by supposedly the same key. In effect the store grows forever and failure rates pile up

[vzarytovskii]

The thing it boils down to is we put something in the CD and later it can't be found by supposedly the same key. In effect the store grows forever and failure rates pile up

This is expected, since cache keys rely on stamp. So any change to type will result in a new stamp.

But those would be misses. I don't get what are evict failures. Eviction is simply "get keys with specific expiration, remove them". If we count removal failures, it means either that they were removed between fetch and remove, or we didn't use the same key.

What does evict success metric look like. And evict attempts. Like how many times we tried to evict, how many keys we decided we want to evict, how many did we successfully evict and unsuccessfully.

When I tested it last time with BDN, I didn't see this behaviour. I saw cache grow and shrink as I would expect it to.

[majocha]

The thing it boils down to is we put something in the CD and later it can't be found by supposedly the same key. In effect the store grows forever and failure rates pile up

This is expected, since cache keys rely on stamp. So any change to type will result in a new stamp.

Yes, and that mutability does not interfere with Equals. However, during eviction we do store.TryRemove(key) and it fails, returns false.

Now, I just tried to find out if the key really is there in such a situation. Evaluating store |> Seq.find (fun kvp -> kvp.Key = key) indicates it is still present, just TryRemove failed to find it.

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.

[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.