diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index 47210a580fd..f2e8661b650 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md @@ -1,5 +1,6 @@ ### Fixed +* Stop leaking a `System.Diagnostics.Metrics.MeterListener` per `Cache` in DEBUG builds. Each cache created a `CacheMetrics.CacheMetricsListener` (which starts a `MeterListener` registered in the process-global metrics registry) and never disposed it, so listeners accumulated for the lifetime of the process. Because every cache hit/miss/add published to all registered listeners, the per-operation cost grew linearly with the number of leaked listeners, so repeated checks (and Debug FCS test runs) slowed down over time. The per-cache `CacheMetricsListener` and the per-instance `cacheId` tag are removed; `DebugDisplay` and tests now read the existing name-aggregated stats populated by the single `ListenToAll` listener, so no per-cache listener is created and no per-operation cost is added. ([PR #19995](https://github.com/dotnet/fsharp/pull/19995)) * Fix state machine lowering dropping the side-effectful receiver of an unused unit-typed member access (e.g. inside `task { (effectful()).UnitProp }`). ([Issue #13099](https://github.com/dotnet/fsharp/issues/13099), [PR #19885](https://github.com/dotnet/fsharp/pull/19885)) * `--deterministic` Release builds now produce byte-identical `FSharp.Compiler.Service.dll` under `--parallelcompilation+` and `--parallelcompilation-`, so it is restored to the determinism gate (now also checked sequential-vs-parallel). Code generation runs the same deferred per-file drain in both modes, with type/member/field emit-order keys and generated names derived from the file being emitted rather than thread-scheduling order. ([Issue #19928](https://github.com/dotnet/fsharp/issues/19928), [PR #19929](https://github.com/dotnet/fsharp/pull/19929)) * Fix `[]` silently producing duplicate IL entries (FS0192/FS2014) when applied to a multi-value let-binding (e.g. `let a, b = 1, 2`); now emits FS0755 at type-check time. ([Issue #6131](https://github.com/dotnet/fsharp/issues/6131), [PR #19924](https://github.com/dotnet/fsharp/pull/19924)) diff --git a/src/Compiler/Service/service.fs b/src/Compiler/Service/service.fs index c0dd6e21d09..3584ca61e49 100644 --- a/src/Compiler/Service/service.fs +++ b/src/Compiler/Service/service.fs @@ -623,9 +623,6 @@ type FSharpChecker static member Instance = globalInstance.Force() - static member internal CreateOverloadCacheMetricsListener() = - new CacheMetrics.CacheMetricsListener("overloadResolutionCache") - member internal _.FrameworkImportsCache = backgroundCompiler.FrameworkImportsCache /// Tokenize a single line, returning token information and a tokenization state represented by an integer diff --git a/src/Compiler/Service/service.fsi b/src/Compiler/Service/service.fsi index ae2b253c676..1584e19562b 100644 --- a/src/Compiler/Service/service.fsi +++ b/src/Compiler/Service/service.fsi @@ -506,9 +506,6 @@ type public FSharpChecker = [] static member Instance: FSharpChecker - /// Creates a listener for overload resolution cache metrics, aggregating across all compilations. - static member internal CreateOverloadCacheMetricsListener: unit -> CacheMetrics.CacheMetricsListener - member internal FrameworkImportsCache: FrameworkImportsCache member internal ReferenceResolver: LegacyReferenceResolver diff --git a/src/Compiler/Utilities/Caches.fs b/src/Compiler/Utilities/Caches.fs index fb024844e09..d8c143cebc6 100644 --- a/src/Compiler/Utilities/Caches.fs +++ b/src/Compiler/Utilities/Caches.fs @@ -22,14 +22,12 @@ module CacheMetrics = let creations = Meter.CreateCounter("creations", "count") let disposals = Meter.CreateCounter("disposals", "count") - let mutable private nextCacheId = 0 - let mkTags (name: string) = - let cacheId = Interlocked.Increment &nextCacheId // Avoid TagList(ReadOnlySpan<...>) to support net472 runtime + // Only the cache name is tagged: a per-instance id would be published on every measurement, + // inflating the tag payload sent to any connected exporter for no in-process benefit. let mutable tags = TagList() tags.Add("name", box name) - tags.Add("cacheId", box cacheId) tags let Add (tags: inref) = adds.Add(1L, &tags) @@ -78,6 +76,10 @@ module CacheMetrics = let getStatsByName name = statsByName.GetOrAdd(name, fun _ -> Stats()) + let getTotalsByName name = (getStatsByName name).GetTotals() + + let getRatioByName name = (getStatsByName name).Ratio + let ListenToAll () = let listener = new MeterListener() @@ -123,50 +125,6 @@ module CacheMetrics = Console.WriteLine(StatsToString()) } - [] - type CacheMetricsListener(cacheTags: TagList, ?nameOnlyFilter: string) = - - let stats = Stats() - let listener = new MeterListener() - - do - for instrument in allCounters do - listener.EnableMeasurementEvents instrument - - listener.SetMeasurementEventCallback(fun instrument v tags _ -> - let shouldIncrement = - match nameOnlyFilter with - | Some filterName -> - match tags[0].Value with - | :? string as name when name = filterName -> true - | _ -> false - | None -> tags[0] = cacheTags[0] && tags[1] = cacheTags[1] - - if shouldIncrement then - stats.Incr instrument.Name v) - - listener.Start() - - /// Creates a listener that aggregates metrics across all cache instances with the given name. - new(cacheName: string) = new CacheMetricsListener(TagList(), nameOnlyFilter = cacheName) - - interface IDisposable with - member _.Dispose() = listener.Dispose() - - /// Gets the current totals for each metric type. - member _.GetTotals() = stats.GetTotals() - - /// Gets the current hit ratio (hits / (hits + misses)). - member _.Ratio = stats.Ratio - - /// Gets the total number of cache hits. - member _.Hits = stats.GetTotals().[hits.Name] - - /// Gets the total number of cache misses. - member _.Misses = stats.GetTotals().[misses.Name] - - override _.ToString() = stats.ToString() - [] type EvictionMode = | NoEviction @@ -361,10 +319,6 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke post, dispose -#if DEBUG - let debugListener = new CacheMetrics.CacheMetricsListener(tags) -#endif - do CacheMetrics.Created &tags member val Evicted = evicted.Publish @@ -430,9 +384,6 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke CacheMetrics.Update &tags post (EvictionQueueMessage.Update result) - member _.CreateMetricsListener() = - new CacheMetrics.CacheMetricsListener(tags) - member _.Dispose() = if Interlocked.Exchange(&disposed, 1) = 0 then disposeEvictionProcessor () @@ -447,5 +398,8 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke override this.Finalize() = this.Dispose() #if DEBUG - member _.DebugDisplay() = debugListener.ToString() + // Shows the totals aggregated for this cache's name. Populated only while a metrics listener + // (CacheMetrics.ListenToAll, e.g. under --times or the editor's metrics view) is running. + member _.DebugDisplay() = + (CacheMetrics.getStatsByName name).ToString() #endif diff --git a/src/Compiler/Utilities/Caches.fsi b/src/Compiler/Utilities/Caches.fsi index 3e1c98e9bb1..e0bff618fcb 100644 --- a/src/Compiler/Utilities/Caches.fsi +++ b/src/Compiler/Utilities/Caches.fsi @@ -8,25 +8,18 @@ module CacheMetrics = /// Global telemetry Meter for all caches. Exposed for testing purposes. /// Set FSHARP_OTEL_EXPORT environment variable to enable OpenTelemetry export to external collectors in tests. val Meter: Meter + + /// Current metric totals aggregated across all cache instances with the given name. + /// Totals only accumulate while a listener from ListenToAll is running. + val internal getTotalsByName: name: string -> Map + + /// Current hit ratio (hits / (hits + misses)) aggregated across all cache instances with the given name. + val internal getRatioByName: name: string -> float + val internal ListenToAll: unit -> IDisposable val internal StatsToString: unit -> string val internal CaptureStatsAndWriteToConsole: unit -> IDisposable - /// A listener that captures cache metrics, matching by cache name or exact cache tags. - [] - type CacheMetricsListener = - /// Creates a listener that aggregates metrics across all cache instances with the given name. - new: cacheName: string -> CacheMetricsListener - /// Gets the current totals for each metric type. - member GetTotals: unit -> Map - /// Gets the current hit ratio (hits / (hits + misses)). - member Ratio: float - /// Gets the total number of cache hits. - member Hits: int64 - /// Gets the total number of cache misses. - member Misses: int64 - interface IDisposable - [] type internal EvictionMode = /// Do not evict items, cache is effectively a ConcurrentDictionary. @@ -74,5 +67,3 @@ type internal Cache<'Key, 'Value when 'Key: not null> = member Evicted: IEvent /// For testing only. member EvictionFailed: IEvent - /// For testing only. Creates a local telemetry listener for this cache instance. - member CreateMetricsListener: unit -> CacheMetrics.CacheMetricsListener diff --git a/tests/FSharp.Compiler.ComponentTests/CompilerService/Caches.fs b/tests/FSharp.Compiler.ComponentTests/CompilerService/Caches.fs index b7aac72a93b..c00f1af81ba 100644 --- a/tests/FSharp.Compiler.ComponentTests/CompilerService/Caches.fs +++ b/tests/FSharp.Compiler.ComponentTests/CompilerService/Caches.fs @@ -16,6 +16,12 @@ let shouldNeverTimeout = 200_000 let defaultStructural() = CacheOptions.getDefault HashIdentity.Structural +// Metrics assertions below read absolute per-name totals via CacheMetrics.getTotalsByName. Those totals +// are aggregated process-globally while a CacheMetrics.ListenToAll() listener is running. This works +// because each test uses a unique cache name and this module is the only ListenToAll caller in the +// assembly, so nothing else increments those names. A second concurrently-active listener would +// double-count every measurement, so keep it that way. + [] let ``Create and dispose many`` () = let caches = @@ -28,8 +34,8 @@ let ``Create and dispose many`` () = [] let ``Basic add and retrieve`` () = let name = "Basic_add_and_retrieve" + use _ = CacheMetrics.ListenToAll() use cache = new Cache(defaultStructural(), name = name) - use metricsListener = cache.CreateMetricsListener() cache.TryAdd("key1", 1) |> shouldBeTrue cache.TryAdd("key2", 2) |> shouldBeTrue @@ -45,14 +51,14 @@ let ``Basic add and retrieve`` () = cache.TryGetValue("key3", &value) |> shouldBeFalse // Metrics assertions - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName name totals.["adds"] |> shouldEqual 2L [] let ``Eviction of least recently used`` () = let name = "Eviction_of_least_recently_used" + use _ = CacheMetrics.ListenToAll() use cache = new Cache({ defaultStructural() with TotalCapacity = 2; HeadroomPercentage = 0 }, name = name) - use metricsListener = cache.CreateMetricsListener() cache.TryAdd("key1", 1) |> shouldBeTrue cache.TryAdd("key2", 2) |> shouldBeTrue @@ -76,7 +82,7 @@ let ``Eviction of least recently used`` () = value |> shouldEqual 3 // Metrics assertions - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName name totals.["adds"] |> shouldEqual 3L [] @@ -85,14 +91,14 @@ let ``Stress test evictions`` () = let iterations = 10_000 let name = "Stress test evictions" + use _ = CacheMetrics.ListenToAll() use cache = new Cache({ defaultStructural() with TotalCapacity = cacheSize; HeadroomPercentage = 0 }, name = name) - use metricsListener = cache.CreateMetricsListener() let evictionsCompleted = new TaskCompletionSource() let expectedEvictions = iterations - cacheSize cache.Evicted.Add <| fun () -> - if metricsListener.GetTotals().["evictions"] = expectedEvictions then + if (CacheMetrics.getTotalsByName name).["evictions"] = expectedEvictions then evictionsCompleted.SetResult() cache.EvictionFailed.Add <| fun _ -> @@ -114,13 +120,14 @@ let ``Stress test evictions`` () = value |> shouldEqual iterations // Metrics assertions - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName name totals.["adds"] |> shouldEqual (int64 iterations) [] let ``Metrics can be retrieved`` () = - use cache = new Cache({ defaultStructural() with TotalCapacity = 2; HeadroomPercentage = 0 }, name = "test_metrics") - use metricsListener = cache.CreateMetricsListener() + let name = "test_metrics" + use _ = CacheMetrics.ListenToAll() + use cache = new Cache({ defaultStructural() with TotalCapacity = 2; HeadroomPercentage = 0 }, name = name) cache.TryAdd("key1", 1) |> shouldBeTrue cache.TryAdd("key2", 2) |> shouldBeTrue @@ -135,17 +142,17 @@ let ``Metrics can be retrieved`` () = cache.TryAdd("key3", 3) |> shouldBeTrue evictionCompleted.Task.Wait shouldNeverTimeout |> shouldBeTrue - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName name - metricsListener.Ratio |> shouldEqual 1.0 + CacheMetrics.getRatioByName name |> shouldEqual 1.0 totals.["evictions"] |> shouldEqual 1L totals.["adds"] |> shouldEqual 3L [] let ``GetOrAdd basic usage`` () = let cacheName = "GetOrAdd_basic_usage" + use _ = CacheMetrics.ListenToAll() use cache = new Cache(defaultStructural(), name = cacheName) - use metricsListener = cache.CreateMetricsListener() let mutable factoryCalls = 0 let factory k = factoryCalls <- factoryCalls + 1; String.length k let v1 = cache.GetOrAdd("abc", factory) @@ -157,17 +164,17 @@ let ``GetOrAdd basic usage`` () = v3 |> shouldEqual 4 factoryCalls |> shouldEqual 2 // Metrics assertions - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName cacheName totals.["hits"] |> shouldEqual 1L totals.["misses"] |> shouldEqual 2L - metricsListener.Ratio |> shouldEqual (1.0/3.0) + CacheMetrics.getRatioByName cacheName |> shouldEqual (1.0/3.0) totals.["adds"] |> shouldEqual 2L [] let ``AddOrUpdate basic usage`` () = let cacheName = "AddOrUpdate_basic_usage" + use _ = CacheMetrics.ListenToAll() use cache = new Cache(defaultStructural(), name = cacheName) - use metricsListener = cache.CreateMetricsListener() cache.AddOrUpdate("x", 1) let mutable value = 0 cache.TryGetValue("x", &value) |> shouldBeTrue @@ -179,10 +186,10 @@ let ``AddOrUpdate basic usage`` () = cache.TryGetValue("y", &value) |> shouldBeTrue value |> shouldEqual 99 // Metrics assertions - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName cacheName totals.["hits"] |> shouldEqual 3L // 3 cache hits totals.["misses"] |> shouldEqual 0L // 0 cache misses - metricsListener.Ratio |> shouldEqual 1.0 + CacheMetrics.getRatioByName cacheName |> shouldEqual 1.0 totals.["adds"] |> shouldEqual 2L // "x" and "y" added totals.["updates"] |> shouldEqual 1L // "x" updated @@ -191,8 +198,8 @@ type BoxedKey = BoxedKey of int * int [] let ``GetOrAdd with reference identity`` () = let cacheName = "GetOrAdd_with_Reference" + use _ = CacheMetrics.ListenToAll() use cache = new Cache(CacheOptions.getReferenceIdentity(), cacheName) - use metricsListener = cache.CreateMetricsListener() let t1 = BoxedKey (1, 2) let t2 = BoxedKey (1, 2) let t3 = BoxedKey (1, 2) @@ -219,17 +226,17 @@ let ``GetOrAdd with reference identity`` () = v1'' |> shouldEqual v1' v2'' |> shouldEqual v2' // Metrics assertions - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName cacheName totals.["hits"] |> shouldEqual 4L totals.["misses"] |> shouldEqual 3L - metricsListener.Ratio |> shouldEqual (4.0 / 7.0) + CacheMetrics.getRatioByName cacheName |> shouldEqual (4.0 / 7.0) totals.["adds"] |> shouldEqual 2L [] let ``AddOrUpdate with reference identity`` () = let cacheName = "AddOrUpdate_with_Reference" + use _ = CacheMetrics.ListenToAll() use cache = new Cache(CacheOptions.getReferenceIdentity(), name = cacheName) - use metricsListener = cache.CreateMetricsListener() let t1 = box (3, 4) let t2 = box (3, 4) cache.AddOrUpdate(t1, 7) @@ -248,9 +255,9 @@ let ``AddOrUpdate with reference identity`` () = cache.TryGetValue(t1, &value1Updated) |> shouldBeTrue value1Updated |> shouldEqual 9 // Metrics assertions - let totals = metricsListener.GetTotals() + let totals = CacheMetrics.getTotalsByName cacheName totals.["hits"] |> shouldEqual 3L // 3 cache hits totals.["misses"] |> shouldEqual 0L // 0 cache misses - metricsListener.Ratio |> shouldEqual 1.0 + CacheMetrics.getRatioByName cacheName |> shouldEqual 1.0 totals.["adds"] |> shouldEqual 2L // t1 and t2 added totals.["updates"] |> shouldEqual 1L // t1 updated once diff --git a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl index 0a8b95305b6..0c81c8df894 100644 --- a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl +++ b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl @@ -2035,16 +2035,6 @@ FSharp.Compiler.AbstractIL.ILBinaryReader: FSharp.Compiler.AbstractIL.ILBinaryRe FSharp.Compiler.AbstractIL.ILBinaryReader: FSharp.Compiler.AbstractIL.ILBinaryReader+MetadataOnlyFlag FSharp.Compiler.AbstractIL.ILBinaryReader: FSharp.Compiler.AbstractIL.ILBinaryReader+ReduceMemoryFlag FSharp.Compiler.AbstractIL.ILBinaryReader: FSharp.Compiler.AbstractIL.ILBinaryReader+Shim -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Double Ratio -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Double get_Ratio() -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Int64 Hits -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Int64 Misses -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Int64 get_Hits() -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Int64 get_Misses() -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Microsoft.FSharp.Collections.FSharpMap`2[System.String,System.Int64] GetTotals() -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: System.String ToString() -FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener: Void .ctor(System.String) -FSharp.Compiler.Caches.CacheMetrics: FSharp.Compiler.Caches.CacheMetrics+CacheMetricsListener FSharp.Compiler.Caches.CacheMetrics: System.Diagnostics.Metrics.Meter Meter FSharp.Compiler.Caches.CacheMetrics: System.Diagnostics.Metrics.Meter get_Meter() FSharp.Compiler.Cancellable: Boolean HasCancellationToken diff --git a/tests/FSharp.Compiler.Service.Tests/OverloadCacheTests.fs b/tests/FSharp.Compiler.Service.Tests/OverloadCacheTests.fs index cf6032afb3a..4abe9d9c46a 100644 --- a/tests/FSharp.Compiler.Service.Tests/OverloadCacheTests.fs +++ b/tests/FSharp.Compiler.Service.Tests/OverloadCacheTests.fs @@ -1,5 +1,10 @@ // Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information. +// These tests are serialized (NotThreadSafeResourceCollection) because they read process-global state: +// the shared language-service `checker`, and the process-global cache metrics that +// `CacheMetrics.ListenToAll` aggregates by name (see the `use _ = CacheMetrics.ListenToAll()` in each +// test). Running them in parallel with each other, or alongside anything else that drives caches while +// a listener is attached, would let counts from unrelated work bleed into the before/after deltas. [] module FSharp.Compiler.Service.Tests.OverloadCacheTests @@ -54,23 +59,31 @@ let generateRepetitiveOverloadCalls (callCount: int) = [] let ``Overload cache hit rate exceeds 70 percent for repetitive int-int calls`` () = - use listener = FSharpChecker.CreateOverloadCacheMetricsListener() + use _ = CacheMetrics.ListenToAll() checker.ClearLanguageServiceRootCachesAndCollectAndFinalizeAllTransients() - + + // Measure only this compilation's activity: the per-name totals are process-global, so snapshot + // before/after and diff rather than reading absolute counts. + let before = CacheMetrics.getTotalsByName "overloadResolutionCache" + let callCount = 150 let source = generateRepetitiveOverloadCalls callCount checkSourceHasNoErrors source |> ignore - - let hits = listener.Hits - let misses = listener.Misses + + let after = CacheMetrics.getTotalsByName "overloadResolutionCache" + let hits = after.["hits"] - before.["hits"] + let misses = after.["misses"] - before.["misses"] Assert.True(hits + misses > 0L, "Expected cache activity but got no hits or misses - is the cache enabled?") - Assert.True(listener.Ratio > 0.70, sprintf "Expected hit ratio > 70%%, but got %.2f%%" (listener.Ratio * 100.0)) + let ratio = float hits / float (hits + misses) + Assert.True(ratio > 0.70, sprintf "Expected hit ratio > 70%%, but got %.2f%%" (ratio * 100.0)) [] let ``Overload cache returns correct resolution`` () = - use listener = FSharpChecker.CreateOverloadCacheMetricsListener() + use _ = CacheMetrics.ListenToAll() checker.ClearLanguageServiceRootCachesAndCollectAndFinalizeAllTransients() - + + let before = CacheMetrics.getTotalsByName "overloadResolutionCache" + let source = """ type Overloaded = static member Process(x: int) = "int" @@ -91,7 +104,9 @@ let f2 = Overloaded.Process(2.0) """ checkSourceHasNoErrors source |> ignore - Assert.True(listener.Hits > 0L, "Expected cache hits for repeated overload calls") + + let after = CacheMetrics.getTotalsByName "overloadResolutionCache" + Assert.True(after.["hits"] - before.["hits"] > 0L, "Expected cache hits for repeated overload calls") let overloadCorrectnessTestCases () : obj[] seq = seq { @@ -273,9 +288,8 @@ let ``Overload resolution correctness`` (_scenario: string, source: string) = [] let ``Overload cache benefits from rigid generic type parameters`` () = - use listener = FSharpChecker.CreateOverloadCacheMetricsListener() checker.ClearLanguageServiceRootCachesAndCollectAndFinalizeAllTransients() - + let source = """ type Assert = static member Equal(expected: int, actual: int) = expected = actual