PoC: Compiler caches #18190

vzarytovskii · 2024-12-30T18:51:18Z

This is a very first naive draft of universal cache for compiler internals.

Currently it's very ~~dumb~~ straightforward - have an underlying concurrent dictionary, evict on every insert.

Things it lacks:

I pretty much YOLO'd the eviction in about 30 minutes or so, and I don't expect it to be efficient.
Weakness handling.
Fair concurrent access (now everything is race-y, which is not inherently bad).
Shortcuts for eviction.
Eviction tasks+cancellation (currently the default strategy is to block for each eviction).
It's not the most memory-efficient soution there is, pretty much same efficiency as ConcurrentDictionaries we use currently.
Probably something else i've forgotten.

It currently solves the only issue versus using ConcurrentDictionarry - eviction. It can evict things, though not in the most efficient way possible.

Personally, I think, that first it has to be at least as good as ConcurrentDictionary+eviction, so we can enable some caching for tooling as well as for compiler.

github-actions · 2024-12-30T18:51:58Z

❗ Release notes required

@vzarytovskii,

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.200.md	No release notes found or release notes format is not correct

vzarytovskii · 2025-01-02T16:09:29Z

I have added a CWT-based cache as well (optional, off by default), but it really needs more testing to make sure nothing is leaking.

T-Gro · 2025-01-03T13:47:25Z

src/Compiler/Utilities/Caches.fs

+            while not cts.Token.IsCancellationRequested do
+                if this.GetEvictCount() > 0 then
+                    this.TryEvictItems ()
+                // do! Task.Delay(100, cts.Token)


Exponential backoff based on number of evicted items?

Did I evict 0 - can afford a longer delay.
A lot of stuff evicted - try again shortly after.

Yes, that was my idea as well

So, I made it based on the cache utilization. It will fluctuate from 0 to 1 seconds. Worst case scenario would be if cache is populated in bursts (and has max size way off), then it will resize. Wondering if it's good enough for now? @0101 @T-Gro @KevinRansom thoughts?

src/Compiler/Utilities/Caches.fs

vzarytovskii · 2025-01-06T13:12:24Z

I have added a CWT-based cache as well (optional, off by default), but it really needs more testing to make sure nothing is leaking.

FYI, I have removed it for time being, since making it part of one Cache object shown to be problematic, since in many places WeakReference or CWT will imply the use of the not struct constraint, which we don't want in generic cache. I will focus on non-weak cache (pretty much as a drop-in replacement for some existing uses of ConcurrentDictionary), and then add WeakCache separately, with keys and values-based weakness logic for places when we need to replace keys on regular basis.

vzarytovskii · 2025-01-06T18:32:26Z

src/Compiler/Utilities/Caches.fs

+    let inline mkDelayedSeq (f: unit -> IEnumerable<'T>) =
+        mkSeq (fun () -> f().GetEnumerator())
+
+    let inline sortBy ([<InlineIfLambda>] projection) (source: ConcurrentDictionary<_, _>) =


Not really sure about InlineIfLambda here.

vzarytovskii · 2025-01-06T18:34:16Z

src/Compiler/Utilities/Caches.fs

+// Default Seq.* function have one issue - when doing `Seq.sortBy`, it will call a `ToArray` on the collection,
+// which is *not* calling `ConcurrentDictionary.ToArray`, but uses a custom one instead (treating it as `ICollection`)
+// this leads to and exception when trying to evict without locking (The index is equal to or greater than the length of the array,
+// or the number of elements in the dictionary is greater than the available space from index to the end of the destination array.)
+// this is casuedby insertions happened between reading the `Count` and doing the `CopyTo`.
+// This solution introduces a custom `ConcurrentDictionary.sortBy` which will be calling a proper `CopyTo`, the one on the ConcurrentDictionary itself.


This is somewhat important to note. There are almost no changes comparing to how Seq.sortBy works, it just calls into ConcurrentDictionary.ToArray, instead of internal toArray (which results in calling Enumerable.ToArray, which is not thread-safe).

src/Compiler/Utilities/Caches.fs

vzarytovskii · 2025-01-06T18:38:21Z

src/Compiler/Utilities/Caches.fs

+                | true, _ -> eviction.Trigger(key)
+                | _ -> () // TODO: We probably want to count eviction misses as well?
+
+    // TODO: Shall this be a safer task, wrapping everything in try .. with, so it's not crashing silently?


Another question here.

If we have some solid tests to be pretty confident it won't crash and will keep doing what it should, then it's probably fine. Also since it's executed with Task.Run, does task vs. backgroundTask make any difference?

vzarytovskii · 2025-01-06T18:43:47Z

src/Compiler/Utilities/Caches.fs

+
+        }
+
+    // TODO: Explore an eviction shortcut, some sort of list of keys to evict first, based on the strategy.


This would be quite easy to do - maintain some structure as shortcut (like linked list), and move pointers to either end of it based on strategy (most accessed to the end, for example), and chop beginning, since we don't care about ordering.

The question is if it's worth to allocate, maintain ans synchronise it.

I would say it's worth it, especially if we want to put more items in the cache. Since eviction is super cheap then it can be done on each access. Also we then wouldn't need the CachedEntity type. At least for the eviction strategies based on recency.

I guess the only downside is that we'd have to maintain the lock to ensure thread safety. Probably wouldn't need a ConcurrentDictionary then also.

Anyway, this can always be done as a future improvement. Ideally if we have some benchmarks for it.

Yeah, I'm not a huge fan of dealing with concurrency myself tbh, when CD exists and is done well. I'll see what can be done with backlog.

src/Compiler/Utilities/Caches.fs

vzarytovskii · 2025-01-13T18:58:30Z

This is more or less ready (baselines need updating, I will do it this week).

This, in my opinion, is good to replace ConcurrentDictionary<_,_> at least in the place where we use it to cache subsumptions. Once it's replaced, we can finally use it in tooling, which will allow to make tooling 5x-10x faster in cases when new (net7+) BCL is involved together with using a bunch of interfaces and overloads (like it was the case with OpenTK library).

Approach here is pretty straightforward, please refer to comments.

cc @KevinRansom, @0101, @T-Gro PTAL, i would love to have it under preview for tooling in 9.0.300.

0101

Looks good. Just needs some basic tests and plugging it into the place where it's needed.

0101 · 2025-01-14T10:05:15Z

src/Compiler/Utilities/Caches.fs

+type Cache<'Key, 'Value> (options: CacheOptions) as this =
+
+    let capacity = options.MaximumCapacity + (options.MaximumCapacity * options.PercentageToEvict / 100)
+    let cts = new CancellationTokenSource()


Is this needed when it never gets cancelled?

I added it in case if there's ever a need for it to get cancelled (via dispose).

vzarytovskii added 2 commits December 30, 2024 19:46

wip

f56b2dc

wip

992c341

vzarytovskii added 4 commits January 2, 2025 14:13

wip

400335b

wip

018ec63

wip

4f4ca13

wip

7ce92e6

vzarytovskii added 4 commits January 2, 2025 17:11

wip

a44dbe5

wip

2c04442

wip

8865d2e

wip

c16bea8

T-Gro reviewed Jan 3, 2025

View reviewed changes

src/Compiler/Utilities/Caches.fs Outdated Show resolved Hide resolved

wip

ae1c065

wip

b40fd5a

vzarytovskii commented Jan 6, 2025

View reviewed changes

src/Compiler/Utilities/Caches.fs Outdated Show resolved Hide resolved

vzarytovskii commented Jan 6, 2025

View reviewed changes

uint64->int64

c36c119

majocha reviewed Jan 6, 2025

View reviewed changes

src/Compiler/Utilities/Caches.fs Outdated Show resolved Hide resolved

vzarytovskii added 4 commits January 8, 2025 19:55

wip

f05240d

wip

0663bb9

wip

b947d17

wip

5224504

charlesroddie mentioned this pull request Jan 10, 2025

Compilation/intellisense: dotnet9 much slower than netstandard2.0 (with timings) #18225

Open

0101 reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC: Compiler caches #18190

PoC: Compiler caches #18190

vzarytovskii commented Dec 30, 2024 •

edited

Loading

github-actions bot commented Dec 30, 2024

vzarytovskii commented Jan 2, 2025

T-Gro Jan 3, 2025

vzarytovskii Jan 3, 2025

vzarytovskii Jan 6, 2025 •

edited

Loading

vzarytovskii commented Jan 6, 2025 •

edited

Loading

vzarytovskii Jan 6, 2025

vzarytovskii Jan 6, 2025 •

edited

Loading

vzarytovskii Jan 6, 2025

0101 Jan 14, 2025

vzarytovskii Jan 6, 2025 •

edited

Loading

0101 Jan 14, 2025

vzarytovskii Jan 14, 2025

vzarytovskii commented Jan 13, 2025 •

edited

Loading

0101 left a comment

0101 Jan 14, 2025

vzarytovskii Jan 14, 2025


		}

		// TODO: Explore an eviction shortcut, some sort of list of keys to evict first, based on the strategy.

PoC: Compiler caches #18190

Are you sure you want to change the base?

PoC: Compiler caches #18190

Conversation

vzarytovskii commented Dec 30, 2024 • edited Loading

github-actions bot commented Dec 30, 2024

❗ Release notes required

vzarytovskii commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzarytovskii Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

vzarytovskii commented Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

vzarytovskii Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzarytovskii Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzarytovskii commented Jan 13, 2025 • edited Loading

0101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzarytovskii commented Dec 30, 2024 •

edited

Loading

vzarytovskii Jan 6, 2025 •

edited

Loading

vzarytovskii commented Jan 6, 2025 •

edited

Loading

vzarytovskii Jan 6, 2025 •

edited

Loading

vzarytovskii Jan 6, 2025 •

edited

Loading

vzarytovskii commented Jan 13, 2025 •

edited

Loading