Avoid leaking a MeterListener per Cache in DEBUG builds#19995
Open
NatElkins wants to merge 1 commit into
Open
Conversation
Contributor
❗ 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
majocha
reviewed
Jun 26, 2026
| recordMetric CacheMetrics.updates | ||
| post (EvictionQueueMessage.Update result) | ||
|
|
||
| member _.CreateMetricsListener() = |
Contributor
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.
majocha
reviewed
Jun 26, 2026
| @@ -32,12 +32,6 @@ module CacheMetrics = | |||
| tags.Add("cacheId", box cacheId) | |||
Contributor
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.
T-Gro
reviewed
Jun 29, 2026
| // 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 |
Member
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.