Eviction processor no longer keeps cache instance alive (#20758) · dotnet/fsharp@5a7448b · GitHub
Skip to content

Commit 5a7448b

Browse files
majochamajocha
authored
Eviction processor no longer keeps cache instance alive (#20758)
* eviction processor no longer keeps cache instance alive * get rid of dispose and finalize * per type singleton * format, simplify type annotations * revert to work item * one GC collect should be enough * add release notes * address review * if there is unexpected error surface it in VS UI * Remove cache eviction isolation test
1 parent 230e3cf commit 5a7448b

5 files changed

Lines changed: 83 additions & 71 deletions

File tree

‎docs/release-notes/.FSharp.Compiler.Service/11.0.100.md‎

Lines changed: 1 addition & 0 deletions

‎src/Compiler/Utilities/Caches.fs‎

Lines changed: 31 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ module CacheMetrics =
2020
let allCounters = [ adds; updates; hits; misses; evictions; evictionFails ]
2121

2222
let creations = Meter.CreateCounter<int64>("creations", "count")
23-
let disposals = Meter.CreateCounter<int64>("disposals", "count")
2423

2524
let mkTags (name: string) =
2625
// Avoid TagList(ReadOnlySpan<...>) to support net472 runtime
@@ -37,7 +36,6 @@ module CacheMetrics =
3736
let Eviction (tags: inref<TagList>) = evictions.Add(1L, &tags)
3837
let EvictionFail (tags: inref<TagList>) = evictionFails.Add(1L, &tags)
3938
let Created (tags: inref<TagList>) = creations.Add(1L, &tags)
40-
let Disposed (tags: inref<TagList>) = disposals.Add(1L, &tags)
4139

4240
type Stats() =
4341
let totals = Map [ for counter in allCounters -> counter.Name, ref 0L ]
@@ -203,10 +201,30 @@ type CachedEntity<'Key, 'Value> =
203201
override this.ToString() = $"{this.Key}"
204202

205203
[<Struct>]
206-
type EvictionQueueMessage<'Entity, 'Target> =
207-
| Add of 'Entity * 'Target
204+
type EvictionQueueMessage<'Entity, 'Store> =
205+
| Add of 'Entity * 'Store
208206
| Update of 'Entity
209207

208+
module EvictionProcessor =
209+
// Shared work queue for all caches running in EvictionMode.MailboxProcessor.
210+
let private agent =
211+
MailboxProcessor.Start(
212+
(fun inbox ->
213+
let rec loop () =
214+
async {
215+
let! work = inbox.Receive()
216+
work ()
217+
return! loop ()
218+
}
219+
220+
loop ()),
221+
cancellationToken = CancellationToken.None
222+
)
223+
224+
let Error = agent.Error
225+
226+
let post = agent.Post
227+
210228
[<Sealed; NoComparison; NoEquality>]
211229
[<DebuggerDisplay("{DebugDisplay()}")>]
212230
type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Key>, ?name) =
@@ -228,7 +246,7 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke
228246
let capacity = options.TotalCapacity - headroom
229247

230248
let mutable store =
231-
ConcurrentDictionary<'Key, CachedEntity<'Key, 'Value>>(Environment.ProcessorCount, capacity, options.Comparer)
249+
ConcurrentDictionary(Environment.ProcessorCount, capacity, options.Comparer)
232250

233251
let evictionQueue = LinkedList<CachedEntity<'Key, 'Value>>()
234252

@@ -237,16 +255,13 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke
237255

238256
let tags = CacheMetrics.mkTags name
239257

240-
// Track disposal state (0 = not disposed, 1 = disposed)
241-
let mutable disposed = 0
242-
243258
let mutable deadKeysCount = 0
244259

245260
// Keys with unreliable identity can prevent eviction, taking up space in the cache.
246261
// In such case we rebuild the store to remove dead keys.
247262
let rebuildStore () =
248263
let newStore =
249-
ConcurrentDictionary<'Key, CachedEntity<'Key, 'Value>>(Environment.ProcessorCount, capacity, options.Comparer)
264+
ConcurrentDictionary(Environment.ProcessorCount, capacity, options.Comparer)
250265

251266
for entity in evictionQueue do
252267
newStore.TryAdd(entity.Key, entity) |> ignore
@@ -286,38 +301,17 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke
286301
// Store updates are not synchronized with evictionQueue. It is possible the entity is no longer in the queue or already added.
287302
| _ -> ()
288303

289-
let startEvictionProcessor ct =
290-
MailboxProcessor.Start(
291-
(fun mb ->
292-
let rec processNext () =
293-
async {
294-
let! message = mb.Receive()
295-
processEvictionMessage message
296-
return! processNext ()
297-
}
298-
299-
processNext ()),
300-
ct
301-
)
302-
303304
let immediate msg =
304305
lock evictionQueue <| fun () -> processEvictionMessage msg
305306

306-
let post, disposeEvictionProcessor =
307-
match options.EvictionMode with
308-
| EvictionMode.NoEviction -> ignore, ignore
309-
| EvictionMode.Immediate -> immediate, ignore
310-
| EvictionMode.MailboxProcessor ->
311-
let cts = new CancellationTokenSource()
312-
let evictionProcessor = startEvictionProcessor cts.Token
313-
let post = evictionProcessor.Post
314-
315-
let dispose () =
316-
cts.Cancel()
317-
cts.Dispose()
318-
evictionProcessor.Dispose()
307+
let queued msg =
308+
EvictionProcessor.post <| fun () -> processEvictionMessage msg
319309

320-
post, dispose
310+
let post =
311+
match options.EvictionMode with
312+
| EvictionMode.NoEviction -> ignore
313+
| EvictionMode.Immediate -> immediate
314+
| EvictionMode.MailboxProcessor -> queued
321315

322316
do CacheMetrics.Created &tags
323317

@@ -384,19 +378,6 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke
384378
CacheMetrics.Update &tags
385379
post (EvictionQueueMessage.Update result)
386380

387-
member _.Dispose() =
388-
if Interlocked.Exchange(&disposed, 1) = 0 then
389-
disposeEvictionProcessor ()
390-
CacheMetrics.Disposed &tags
391-
392-
interface IDisposable with
393-
member this.Dispose() =
394-
this.Dispose()
395-
GC.SuppressFinalize(this)
396-
397-
// Finalizer to ensure eviction loop is cancelled if Dispose wasn't called.
398-
override this.Finalize() = this.Dispose()
399-
400381
#if DEBUG
401382
// Shows the totals aggregated for this cache's name. Populated only while a metrics listener
402383
// (CacheMetrics.ListenToAll, e.g. under --times or the editor's metrics view) is running.

‎src/Compiler/Utilities/Caches.fsi‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ type internal EvictionMode =
2626
| NoEviction
2727
/// Evict items immediately on the caller's thread when adding a new item that would exceed capacity.
2828
| Immediate
29-
/// Evict items in the background using a MailboxProcessor to queue eviction requests. This may lag behind during heavy load but avoids blocking callers.
29+
/// Evict items in the background using a shared MailboxProcessor to queue eviction requests. This may lag behind during heavy load but avoids blocking callers.
3030
| MailboxProcessor
3131

3232
[<Struct; RequireQualifiedAccess; NoComparison; NoEquality>]
@@ -53,6 +53,9 @@ module internal CacheOptions =
5353
/// Set eviction mode to NoEviction.
5454
val withNoEviction: CacheOptions<'Key> -> CacheOptions<'Key>
5555

56+
module internal EvictionProcessor =
57+
val Error: IEvent<exn>
58+
5659
[<Sealed; NoComparison; NoEquality>]
5760
type internal Cache<'Key, 'Value when 'Key: not null> =
5861
new: options: CacheOptions<'Key> * ?name: string -> Cache<'Key, 'Value>
@@ -61,8 +64,6 @@ type internal Cache<'Key, 'Value when 'Key: not null> =
6164
member GetOrAdd: key: 'Key * valueFactory: ('Key -> 'Value) -> 'Value
6265
member AddOrUpdate: key: 'Key * value: 'Value -> unit
6366

64-
interface IDisposable
65-
6667
/// For testing only.
6768
member Evicted: IEvent<unit>
6869
/// For testing only.

‎tests/FSharp.Compiler.ComponentTests/CompilerService/Caches.fs‎

Lines changed: 38 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
module internal CompilerService.Caches
1+
module CompilerService.Caches
22

33
open FSharp.Compiler.Caches
44
open Xunit
@@ -14,28 +14,48 @@ let shouldNeverTimeout = 15_000
1414
let shouldNeverTimeout = 200_000
1515
#endif
1616

17-
let defaultStructural() = CacheOptions.getDefault HashIdentity.Structural
17+
let private defaultStructural() = CacheOptions.getDefault HashIdentity.Structural
18+
19+
[<System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)>]
20+
let private createIdleCache useMailbox =
21+
let options =
22+
{ defaultStructural() with
23+
TotalCapacity = 1
24+
HeadroomPercentage = 0
25+
EvictionMode = if useMailbox then EvictionMode.MailboxProcessor else EvictionMode.Immediate }
26+
let cache = new Cache<string, obj>(options, name = "Idle cache collection")
27+
let payload = obj()
28+
let evicted = TaskCompletionSource<unit>()
29+
cache.Evicted.Add evicted.SetResult
30+
cache.TryAdd("first", obj()) |> shouldBeTrue
31+
cache.TryAdd("last", payload) |> shouldBeTrue
32+
evicted.Task.Wait shouldNeverTimeout |> shouldBeTrue
33+
struct {| Cache = WeakReference(cache); Payload = WeakReference(payload) |}
34+
35+
[<Theory>]
36+
[<InlineData(false)>]
37+
[<InlineData(true)>]
38+
let ``Idle cache and contents are collected without explicit disposal`` useMailbox =
39+
let references = createIdleCache useMailbox
40+
GC.Collect()
41+
let collected =
42+
System.Threading.SpinWait.SpinUntil(
43+
(fun () -> not references.Cache.IsAlive && not references.Payload.IsAlive),
44+
shouldNeverTimeout)
45+
collected |> shouldBeTrue
1846

1947
// Metrics assertions below read absolute per-name totals via CacheMetrics.getTotalsByName. Those totals
2048
// are aggregated process-globally while a CacheMetrics.ListenToAll() listener is running. This works
2149
// because each test uses a unique cache name and this module is the only ListenToAll caller in the
2250
// assembly, so nothing else increments those names. A second concurrently-active listener would
2351
// double-count every measurement, so keep it that way.
2452

25-
[<Fact>]
26-
let ``Create and dispose many`` () =
27-
let caches =
28-
[ for _ in 1 .. 100 do
29-
new Cache<string, int>(defaultStructural(), name = "Create and dispose many") :> IDisposable ]
30-
31-
for c in caches do
32-
c.Dispose()
3353

3454
[<Fact>]
3555
let ``Basic add and retrieve`` () =
3656
let name = "Basic_add_and_retrieve"
3757
use _ = CacheMetrics.ListenToAll()
38-
use cache = new Cache<string, int>(defaultStructural(), name = name)
58+
let cache = new Cache<string, int>(defaultStructural(), name = name)
3959

4060
cache.TryAdd("key1", 1) |> shouldBeTrue
4161
cache.TryAdd("key2", 2) |> shouldBeTrue
@@ -58,7 +78,7 @@ let ``Basic add and retrieve`` () =
5878
let ``Eviction of least recently used`` () =
5979
let name = "Eviction_of_least_recently_used"
6080
use _ = CacheMetrics.ListenToAll()
61-
use cache = new Cache<string, int>({ defaultStructural() with TotalCapacity = 2; HeadroomPercentage = 0 }, name = name)
81+
let cache = new Cache<string, int>({ defaultStructural() with TotalCapacity = 2; HeadroomPercentage = 0 }, name = name)
6282

6383
cache.TryAdd("key1", 1) |> shouldBeTrue
6484
cache.TryAdd("key2", 2) |> shouldBeTrue
@@ -92,7 +112,7 @@ let ``Stress test evictions`` () =
92112
let name = "Stress test evictions"
93113

94114
use _ = CacheMetrics.ListenToAll()
95-
use cache = new Cache<string, int>({ defaultStructural() with TotalCapacity = cacheSize; HeadroomPercentage = 0 }, name = name)
115+
let cache = new Cache<string, int>({ defaultStructural() with TotalCapacity = cacheSize; HeadroomPercentage = 0 }, name = name)
96116

97117
let evictionsCompleted = new TaskCompletionSource<unit>()
98118
let expectedEvictions = iterations - cacheSize
@@ -127,7 +147,7 @@ let ``Stress test evictions`` () =
127147
let ``Metrics can be retrieved`` () =
128148
let name = "test_metrics"
129149
use _ = CacheMetrics.ListenToAll()
130-
use cache = new Cache<string, int>({ defaultStructural() with TotalCapacity = 2; HeadroomPercentage = 0 }, name = name)
150+
let cache = new Cache<string, int>({ defaultStructural() with TotalCapacity = 2; HeadroomPercentage = 0 }, name = name)
131151

132152
cache.TryAdd("key1", 1) |> shouldBeTrue
133153
cache.TryAdd("key2", 2) |> shouldBeTrue
@@ -152,7 +172,7 @@ let ``Metrics can be retrieved`` () =
152172
let ``GetOrAdd basic usage`` () =
153173
let cacheName = "GetOrAdd_basic_usage"
154174
use _ = CacheMetrics.ListenToAll()
155-
use cache = new Cache<string, int>(defaultStructural(), name = cacheName)
175+
let cache = new Cache<string, int>(defaultStructural(), name = cacheName)
156176
let mutable factoryCalls = 0
157177
let factory k = factoryCalls <- factoryCalls + 1; String.length k
158178
let v1 = cache.GetOrAdd("abc", factory)
@@ -174,7 +194,7 @@ let ``GetOrAdd basic usage`` () =
174194
let ``AddOrUpdate basic usage`` () =
175195
let cacheName = "AddOrUpdate_basic_usage"
176196
use _ = CacheMetrics.ListenToAll()
177-
use cache = new Cache<string, int>(defaultStructural(), name = cacheName)
197+
let cache = new Cache<string, int>(defaultStructural(), name = cacheName)
178198
cache.AddOrUpdate("x", 1)
179199
let mutable value = 0
180200
cache.TryGetValue("x", &value) |> shouldBeTrue
@@ -199,7 +219,7 @@ type BoxedKey = BoxedKey of int * int
199219
let ``GetOrAdd with reference identity`` () =
200220
let cacheName = "GetOrAdd_with_Reference"
201221
use _ = CacheMetrics.ListenToAll()
202-
use cache = new Cache<BoxedKey, int>(CacheOptions.getReferenceIdentity(), cacheName)
222+
let cache = new Cache<BoxedKey, int>(CacheOptions.getReferenceIdentity(), cacheName)
203223
let t1 = BoxedKey (1, 2)
204224
let t2 = BoxedKey (1, 2)
205225
let t3 = BoxedKey (1, 2)
@@ -236,7 +256,7 @@ let ``GetOrAdd with reference identity`` () =
236256
let ``AddOrUpdate with reference identity`` () =
237257
let cacheName = "AddOrUpdate_with_Reference"
238258
use _ = CacheMetrics.ListenToAll()
239-
use cache = new Cache<obj, int>(CacheOptions.getReferenceIdentity(), name = cacheName)
259+
let cache = new Cache<obj, int>(CacheOptions.getReferenceIdentity(), name = cacheName)
240260
let t1 = box (3, 4)
241261
let t2 = box (3, 4)
242262
cache.AddOrUpdate(t1, 7)

‎vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs‎

Lines changed: 9 additions & 0 deletions

0 commit comments

Comments
 (0)