Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/release-notes/.FSharp.Compiler.Service/11.0.100.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
### Fixed

* Stop leaking a `System.Diagnostics.Metrics.MeterListener` per `Cache` in DEBUG builds. Each cache created a `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 `DebugDisplay` totals are now tracked directly instead of via a `MeterListener`. ([PR #19995](https://github.com/dotnet/fsharp/pull/19995))
* Semantic classification no longer marks recursive object self-references (`as this`, `let rec` self-refs) as mutable. ([Issue #5229](https://github.com/dotnet/fsharp/issues/5229))
* Fix `MethodAccessException` under `--realsig+` when a closure (inner `let rec`, `task`/`async` state machine, or quotation splice) inside a member defined in an intrinsic type augmentation (`type C with member ...`) accesses a `private` member of `C`. The synthesized closure is now nested inside the declaring type instead of beside it in the module class. ([Issue #19933](https://github.com/dotnet/fsharp/issues/19933), [PR #19955](https://github.com/dotnet/fsharp/pull/19955))
* Preserve source range for type errors on empty-bodied computation expressions (e.g. `foo {}`) in pipelines, function arguments, and type-annotated contexts, instead of reporting `unknown(1,1)`. ([Issue #19550](https://github.com/dotnet/fsharp/issues/19550), [PR #19849](https://github.com/dotnet/fsharp/pull/19849))
Expand Down
49 changes: 28 additions & 21 deletions src/Compiler/Utilities/Caches.fs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ module CacheMetrics =
tags.Add("cacheId", box cacheId)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get rid of this cacheId also. This is used only to filter per-instance and without doubt bogs things down when any exporter is connected.

tags

let Add (tags: inref<TagList>) = adds.Add(1L, &tags)
let Update (tags: inref<TagList>) = updates.Add(1L, &tags)
let Hit (tags: inref<TagList>) = hits.Add(1L, &tags)
let Miss (tags: inref<TagList>) = misses.Add(1L, &tags)
let Eviction (tags: inref<TagList>) = evictions.Add(1L, &tags)
let EvictionFail (tags: inref<TagList>) = evictionFails.Add(1L, &tags)
let Created (tags: inref<TagList>) = creations.Add(1L, &tags)
let Disposed (tags: inref<TagList>) = disposals.Add(1L, &tags)

Expand Down Expand Up @@ -279,6 +273,23 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke

let tags = CacheMetrics.mkTags name

// Per-cache metric totals used by DebugDisplay. These are incremented directly (see recordMetric)
// rather than via a per-cache CacheMetrics.CacheMetricsListener: a listener starts a
// System.Diagnostics.Metrics.MeterListener which registers in the process-global metrics registry,
// so one per Cache would never be disposed (leak) and would make every measurement O(number of
// caches) as listeners accumulate.
#if DEBUG
let debugStats = CacheMetrics.Stats()
#endif

// Publish one measurement to the global Meter counter (for --times / StatsToString) and, in DEBUG,
// record it in this cache's own totals for DebugDisplay.
let recordMetric (counter: Counter<int64>) =
counter.Add(1L, &tags)
#if DEBUG
debugStats.Incr counter.Name 1L
#endif

// Track disposal state (0 = not disposed, 1 = disposed)
let mutable disposed = 0

Expand Down Expand Up @@ -310,10 +321,10 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke

match store.TryRemove(first.Value.Key) with
| true, _ ->
CacheMetrics.Eviction &tags
recordMetric CacheMetrics.evictions
evicted.Trigger()
| _ ->
CacheMetrics.EvictionFail &tags
recordMetric CacheMetrics.evictionFails
evictionFailed.Trigger()
deadKeysCount <- deadKeysCount + 1

Expand Down Expand Up @@ -361,10 +372,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
Expand All @@ -373,12 +380,12 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke
member _.TryGetValue(key: 'Key, value: outref<'Value>) =
match store.TryGetValue(key) with
| true, entity ->
CacheMetrics.Hit &tags
recordMetric CacheMetrics.hits
post (EvictionQueueMessage.Update entity)
value <- entity.Value
true
| _ ->
CacheMetrics.Miss &tags
recordMetric CacheMetrics.misses
value <- Unchecked.defaultof<'Value>
false

Expand All @@ -388,7 +395,7 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke
let added = store.TryAdd(key, entity)

if added then
CacheMetrics.Add &tags
recordMetric CacheMetrics.adds
post (EvictionQueueMessage.Add(entity, store))

added
Expand All @@ -405,11 +412,11 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke

if wasMiss then
post (EvictionQueueMessage.Add(result, store))
CacheMetrics.Add &tags
CacheMetrics.Miss &tags
recordMetric CacheMetrics.adds
recordMetric CacheMetrics.misses
else
post (EvictionQueueMessage.Update result)
CacheMetrics.Hit &tags
recordMetric CacheMetrics.hits

result.Value

Expand All @@ -424,10 +431,10 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke

// Returned value tells us if the entity was added or updated.
if Object.ReferenceEquals(addValue, result) then
CacheMetrics.Add &tags
recordMetric CacheMetrics.adds
post (EvictionQueueMessage.Add(addValue, store))
else
CacheMetrics.Update &tags
recordMetric CacheMetrics.updates
post (EvictionQueueMessage.Update result)

member _.CreateMetricsListener() =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CacheMetricsListener is used only in tests and it just wraps Stats. It would be great to get rid of it.

Expand All @@ -447,5 +454,5 @@ type Cache<'Key, 'Value when 'Key: not null> internal (options: CacheOptions<'Ke
override this.Finalize() = this.Dispose()

#if DEBUG
member _.DebugDisplay() = debugListener.ToString()
member _.DebugDisplay() = debugStats.ToString()
#endif
Loading