fix: clean up profilers for discarded transactions (#3154)

Author: armcknight

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Fixes
+
+- Reclaim memory used by profiler when transactions are discarded (#3154)
+
 ## 8.9.2
 
 ### Improvements

Sentry.xcodeproj/project.pbxproj

@@ -1827,13 +1827,6 @@
 /* End PBXFrameworksBuildPhase section */
 
 /* Begin PBXGroup section */
-		035E73C627D5661A005EEB11 /* Profiling */ = {
-			isa = PBXGroup;
-			children = (
-			);
-			path = Profiling;
-			sourceTree = "<group>";
-		};
 		0A9BF4E028A114690068D266 /* ViewHierarchy */ = {
 			isa = PBXGroup;
 			children = (
@@ -2255,7 +2248,6 @@
 				7BD7299B24654CD500EA3610 /* Helper */,
 				7B944FA924697E9700A10721 /* Integrations */,
 				7BBD18AF24517E5D00427C76 /* Networking */,
-				035E73C627D5661A005EEB11 /* Profiling */,
 				7B3D0474249A3D5800E106B6 /* Protocol */,
 				63FE71D220DA66C500CDBAE8 /* SentryCrash */,
 				7B944FAC2469B41600A10721 /* State */,

SentryTestUtils/ClearTestState.swift

@@ -40,7 +40,7 @@ class TestCleanup: NSObject {
 
 #if os(iOS) || os(macOS) || targetEnvironment(macCatalyst)
         SentryProfiler.getCurrent().stop(for: .normal)
-        SentryTracer.resetConcurrencyTracking()
+        SentryProfiler.resetConcurrencyTracking()
 #endif // os(iOS) || os(macOS) || targetEnvironment(macCatalyst)
 
         #if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst)

SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h

@@ -8,6 +8,12 @@
 #    import "SentryUIViewControllerPerformanceTracker.h"
 #endif // SENTRY_HAS_UIKIT
 
+#import "SentryProfilingConditionals.h"
+
+#if SENTRY_TARGET_PROFILING_SUPPORTED
+#    import "SentryProfiler+Test.h"
+#endif // SENTRY_TARGET_PROFILING_SUPPORTED
+
 #import "PrivateSentrySDKOnly.h"
 #import "SentryAppState.h"
 #import "SentryClient+Private.h"
@@ -26,7 +32,6 @@
 #import "SentryNSTimerFactory.h"
 #import "SentryNetworkTracker.h"
 #import "SentryPerformanceTracker+Testing.h"
-#import "SentryProfiler+Test.h"
 #import "SentryRandom.h"
 #import "SentrySDK+Private.h"
 #import "SentrySDK+Tests.h"

Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm

@@ -16,19 +16,42 @@
 #    endif // SENTRY_HAS_UIKIT
 
 /**
- * a mapping of profilers to the tracers that started them that are still in-flight and will need to
- * query them for their profiling data when they finish. this helps resolve the incongruity between
- * the different timeout durations between tracers (500s) and profilers (30s), where a transaction
- * may start a profiler that then times out, and then a new transaction starts a new profiler, and
- * we must keep the aborted one around until its associated transaction finishes.
+ * a mapping of profilers to the number of tracers that started them that are still in-flight and
+ * will need to query them for their profiling data when they finish. this helps resolve the
+ * incongruity between the different timeout durations between tracers (500s) and profilers (30s),
+ * where a transaction may start a profiler that then times out, and then a new transaction starts a
+ * new profiler, and we must keep the aborted one around until its associated transaction finishes.
  */
 static NSMutableDictionary</* SentryProfiler.profileId */ NSString *,
-    NSMutableSet<SentryTracer *> *> *_gProfilersToTracers;
+    /* number of in-flight tracers */ NSNumber *> *_gProfilersToTracers;
 
 /** provided for fast access to a profiler given a tracer */
 static NSMutableDictionary</* SentryTracer.tracerId */ NSString *, SentryProfiler *>
     *_gTracersToProfilers;
 
+namespace {
+
+/**
+ * Remove a profiler from tracking given the id of the tracer it's associated with.
+ * @warning Must be called from a synchronized context.
+ */
+void
+_unsafe_cleanUpProfiler(SentryProfiler *profiler, NSString *tracerKey)
+{
+    const auto profilerKey = profiler.profileId.sentryIdString;
+
+    [_gTracersToProfilers removeObjectForKey:tracerKey];
+    _gProfilersToTracers[profilerKey] = @(_gProfilersToTracers[profilerKey].unsignedIntValue - 1);
+    if ([_gProfilersToTracers[profilerKey] unsignedIntValue] == 0) {
+        [_gProfilersToTracers removeObjectForKey:profilerKey];
+        if ([profiler isRunning]) {
+            [profiler stopForReason:SentryProfilerTruncationReasonNormal];
+        }
+    }
+}
+
+} // namespace
+
 std::mutex _gStateLock;
 
 void
@@ -48,22 +71,39 @@
 
     if (_gProfilersToTracers == nil) {
         _gProfilersToTracers = [NSMutableDictionary</* SentryProfiler.profileId */ NSString *,
-            NSMutableSet<SentryTracer *> *> dictionaryWithObject:[NSMutableSet setWithObject:tracer]
-                                                          forKey:profilerKey];
+            /* number of in-flight tracers */ NSNumber *>
+            dictionary];
         _gTracersToProfilers =
             [NSMutableDictionary</* SentryTracer.tracerId */ NSString *, SentryProfiler *>
-                dictionaryWithObject:profiler
-                              forKey:tracerKey];
-        return;
+                dictionary];
     }
 
-    if (_gProfilersToTracers[profilerKey] == nil) {
-        _gProfilersToTracers[profilerKey] = [NSMutableSet setWithObject:tracer];
-    } else {
-        [_gProfilersToTracers[profilerKey] addObject:tracer];
+    _gProfilersToTracers[profilerKey] = @(_gProfilersToTracers[profilerKey].unsignedIntValue + 1);
+    _gTracersToProfilers[tracerKey] = profiler;
+}
+
+void
+discardProfilerForTracer(SentryTracer *tracer)
+{
+    std::lock_guard<std::mutex> l(_gStateLock);
+
+    SENTRY_CASSERT(_gTracersToProfilers != nil && _gProfilersToTracers != nil,
+        @"Structures should have already been initialized by the time they are being queried");
+
+    const auto tracerKey = tracer.traceId.sentryIdString;
+    const auto profiler = _gTracersToProfilers[tracerKey];
+
+    if (profiler == nil) {
+        return;
     }
 
-    _gTracersToProfilers[tracerKey] = profiler;
+    _unsafe_cleanUpProfiler(profiler, tracerKey);
+
+#    if SENTRY_HAS_UIKIT
+    if (_gProfilersToTracers.count == 0) {
+        [SentryDependencyContainer.sharedInstance.framesTracker resetProfilingTimestamps];
+    }
+#    endif // SENTRY_HAS_UIKIT
 }
 
 SentryProfiler *_Nullable profilerForFinishedTracer(SentryTracer *tracer)
@@ -81,16 +121,7 @@
         return nil;
     }
 
-    const auto profilerKey = profiler.profileId.sentryIdString;
-
-    [_gTracersToProfilers removeObjectForKey:tracerKey];
-    [_gProfilersToTracers[profilerKey] removeObject:tracer];
-    if ([_gProfilersToTracers[profilerKey] count] == 0) {
-        [_gProfilersToTracers removeObjectForKey:profilerKey];
-        if ([profiler isRunning]) {
-            [profiler stopForReason:SentryProfilerTruncationReasonNormal];
-        }
-    }
+    _unsafe_cleanUpProfiler(profiler, tracerKey);
 
 #    if SENTRY_HAS_UIKIT
     profiler._screenFrameData =
@@ -111,6 +142,13 @@
     [_gTracersToProfilers removeAllObjects];
     [_gProfilersToTracers removeAllObjects];
 }
+
+NSUInteger
+currentProfiledTracers()
+{
+    std::lock_guard<std::mutex> l(_gStateLock);
+    return [_gTracersToProfilers count];
+}
 #    endif // defined(TEST) || defined(TESTCI)
 
 #endif // SENTRY_TARGET_PROFILING_SUPPORTED

Sources/Sentry/SentryProfiler.mm

@@ -515,6 +515,19 @@ + (SentryProfiler *)getCurrentProfiler
 {
     return _gCurrentProfiler;
 }
+
+// this just calls through to SentryProfiledTracerConcurrency.resetConcurrencyTracking(). we have to
+// do this through SentryTracer because SentryProfiledTracerConcurrency cannot be included in test
+// targets via ObjC bridging headers because it contains C++.
++ (void)resetConcurrencyTracking
+{
+    resetConcurrencyTracking();
+}
+
++ (NSUInteger)currentProfiledTracers
+{
+    return currentProfiledTracers();
+}
 #    endif // defined(TEST) || defined(TESTCI)
 
 @end

Sources/Sentry/SentryTracer.m

@@ -172,6 +172,15 @@ - (instancetype)initWithTransactionContext:(SentryTransactionContext *)transacti
     return self;
 }
 
+- (void)dealloc
+{
+#if SENTRY_TARGET_PROFILING_SUPPORTED
+    if (self.isProfiling) {
+        discardProfilerForTracer(self);
+    }
+#endif // SENTRY_TARGET_PROFILING_SUPPORTED
+}
+
 - (nullable SentryTracer *)tracer
 {
     return self;
@@ -831,16 +840,6 @@ - (NSDate *)originalStartTimestamp
     return _startTimeChanged ? _originalStartTimestamp : self.startTimestamp;
 }
 
-#if SENTRY_TARGET_PROFILING_SUPPORTED && (defined(TEST) || defined(TESTCI))
-// this just calls through to SentryProfiledTracerConcurrency.resetConcurrencyTracking(). we have to
-// do this through SentryTracer because SentryProfiledTracerConcurrency cannot be included in test
-// targets via ObjC bridging headers because it contains C++.
-+ (void)resetConcurrencyTracking
-{
-    resetConcurrencyTracking();
-}
-#endif // SENTRY_TARGET_PROFILING_SUPPORTED && (defined(TEST) || defined(TESTCI))
-
 @end
 
 NS_ASSUME_NONNULL_END

Sources/Sentry/include/SentryProfiledTracerConcurrency.h

@@ -17,6 +17,12 @@ SENTRY_EXTERN_C_BEGIN
  */
 void trackProfilerForTracer(SentryProfiler *profiler, SentryTracer *tracer);
 
+/**
+ * For transactions that will be discarded, clean up the bookkeeping state associated with them to
+ * reclaim the memory they're using.
+ */
+void discardProfilerForTracer(SentryTracer *tracer);
+
 /**
  * Return the profiler instance associated with the tracer. If it was the last tracer for the
  * associated profiler, stop that profiler. Copy any recorded @c SentryScreenFrames data into the
@@ -27,6 +33,7 @@ SentryProfiler *_Nullable profilerForFinishedTracer(SentryTracer *tracer);
 
 #    if defined(TEST) || defined(TESTCI)
 void resetConcurrencyTracking(void);
+NSUInteger currentProfiledTracers(void);
 #    endif // defined(TEST) || defined(TESTCI)
 
 SENTRY_EXTERN_C_END