Avoid leaking a MeterListener per Cache in DEBUG builds#19995
Avoid leaking a MeterListener per Cache in DEBUG builds#19995NatElkins wants to merge 5 commits into
Conversation
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
In DEBUG builds, every Cache instance created a CacheMetrics.CacheMetricsListener, which starts a System.Diagnostics.Metrics.MeterListener registered in the process-global metrics registry. These were never disposed, so they accumulated for the lifetime of the process. Because every cache hit/miss/add publishes a measurement to all registered listeners, the per-operation cost grew linearly with the number of leaked listeners, so workloads that create many caches (for example repeated ParseAndCheckProject / per-file checks) slowed down steadily. Track the per-cache totals used by DebugDisplay directly, incrementing a small Stats object alongside the existing global Meter counters, instead of via a per-cache MeterListener. No listener is created, so nothing leaks, and DebugDisplay still works. The now-unused CacheMetrics.Hit/Miss/Add/Update/Eviction/EvictionFail helpers are replaced by a single recordMetric helper.
NatElkins
force-pushed
the
fix-cachemetrics-listener-leak
branch
from
c3e009a to
f96b70e
Compare
github-actions
Bot
added
the
AI-Tooling-Check-Scanned-Clean
| recordMetric CacheMetrics.updates | ||
| post (EvictionQueueMessage.Update result) | ||
|
|
||
| member _.CreateMetricsListener() = |
There was a problem hiding this comment.
CacheMetricsListener is used only in tests and it just wraps Stats. It would be great to get rid of it.
There was a problem hiding this comment.
Done, removed it. That per-cache listener was the leak: each Cache created one via the #if DEBUG debugListener and never disposed it, so dropping the type fixes both at once. DebugDisplay and the cache tests now read the name-aggregated stats from the single ListenToAll listener via getTotalsByName/getRatioByName. Removed CreateOverloadCacheMetricsListener too.
| @@ -32,12 +32,6 @@ module CacheMetrics = | |||
| tags.Add("cacheId", box cacheId) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done, cacheId and nextCacheId are gone. Measurements now carry just the name tag, so nothing filters per instance anymore and the payload sent to any connected exporter is a little smaller.
| // 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 |
There was a problem hiding this comment.
This is only needed when developing features that interact with the caches.
If the impact is so bad, I would not oppose adding a different directive here (DEBUG_COMPILER_CACHES) to reduce the blast radius.
Even though it has been really useful in investigating strange perf constellations.
There was a problem hiding this comment.
Ended up not needing a directive. The per-cache stats tracking this was about is gone entirely, so there is no per-op cost in DEBUG (or any config) left to gate. Stats only accumulate while a ListenToAll listener is attached (--times, the editor metrics view, or a test), so it is opt-in by lifetime. Happy to add DEBUG_COMPILER_CACHES anyway if you would still like the knob.
- Remove the CacheMetrics.CacheMetricsListener type. Its only per-cache use was the #if DEBUG debugListener each Cache created and never disposed, which was the leak this PR set out to fix. (majocha) - Drop the per-instance cacheId tag (and nextCacheId). Measurements now carry only the cache name, shrinking the payload published to any connected exporter and removing the per-instance filtering that was cacheId's only purpose. (majocha) - DebugDisplay and the cache tests read the existing name-aggregated stats via CacheMetrics.getTotalsByName / getRatioByName, populated by the single process-wide ListenToAll listener. No per-cache listener is created and no per-operation cost is added in any configuration, so there is no DEBUG-only overhead left to gate behind a separate directive. (T-Gro) - Overload-cache tests enable ListenToAll and snapshot totals before/after to stay scoped to their own compilation; FSharpChecker .CreateOverloadCacheMetricsListener is removed.
CacheMetricsListener was a public type, so dropping it changes the recorded public surface. Remove its 10 entries from FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl; the SurfaceArea test now passes. Also note the single-listener assumption the cache metric tests rely on.
| [<Fact>] | ||
| let ``Overload cache hit rate exceeds 70 percent for repetitive int-int calls`` () = | ||
| use listener = FSharpChecker.CreateOverloadCacheMetricsListener() | ||
| use _ = CacheMetrics.ListenToAll() |
There was a problem hiding this comment.
This is the global state that makes OverloadCacheTests not thread safe / not parallelizable. It seems there was no comment about it. Would be nice to have it clarified for future readers :)
| // 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() |
There was a problem hiding this comment.
Previously each instance held its own stats via a listener (bad). The Stats object is lighter and could be used directly to keep the functionality of per instance stats.
3 participants
Summary
In
DEBUGbuilds,Cache(src/Compiler/Utilities/Caches.fs) created aCacheMetrics.CacheMetricsListenerper instance. Each listener starts aSystem.Diagnostics.Metrics.MeterListenerthat registers in the process-global metrics registry and was never disposed, so the listeners accumulated for the lifetime of the process.Because every cache hit/miss/add publishes a measurement to all registered listeners, the cost of each cache operation grows linearly with the number of leaked listeners. Workloads that create many caches over time (repeated
ParseAndCheckProject, per-file checks, long IDE sessions, FCS test runs) therefore slow down steadily.How it shows up
I found this while measuring repeated project checks against a Debug FCS. Driving
ParseAndCheckProjecton the same project across 10 edits (only one file changed each time, so the incremental check should be roughly constant):ComputeProjectExtras)A heap dump after those 10 edits showed the accumulation directly:
System.Diagnostics.Metrics.MeterListenerCacheMetrics.CacheMetricsListenerSystem.Diagnostics.DiagNode<...Instrument>+ 98,028DiagNode<...ListenerSubscription>(the global metrics subscription lists)The obvious suspects were ruled out first: cache size factor (1 vs 100 both grow), the project snapshot, a fresh
FSharpCheckerper edit (still grows, so it is process-global), and GC (a forced full collection each check, still grows). The dump named the accumulator.Fix
The per-cache
CacheMetricsListenerexisted only to backDebugDisplay(). Instead of standing up aMeterListenerper cache, the cache now tracks those totals directly in a smallStatsobject, incremented alongside the existing globalMetercounters via a singlerecordMetrichelper. NoMeterListeneris created, so nothing leaks and there is no per-operation "publish to all listeners" cost.DebugDisplay()keeps working with no configuration, and Release builds are unchanged. The now-unusedCacheMetrics.Hit/Miss/Add/Update/Eviction/EvictionFailhelpers are removed in favour ofrecordMetric.With the listener removed, the same loop is flat:
Notes
#if DEBUG, so Release builds were already unaffected. The impact is on Debug FCS: the compiler-dev inner loop, profiling accuracy (a Debug profile is dominated by the metrics publish-to-all-listeners cost), and Debug FCS test runs.mainand is independent of any feature work. The per-cachedebugListenerwas introduced in Print cache stats with --times #18930.