From cd6d52191a4c14c742e0cf4ed62245977d97ec96 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Fri, 14 Jul 2023 13:48:44 -0800 Subject: [PATCH 01/15] rename SentryTracerConcurrency -> SentryProfiledTracerConcurrency --- Sentry.xcodeproj/project.pbxproj | 18 +++++++++--------- .../SentryProfiledTracerConcurrency.h} | 0 .../SentryProfiledTracerConcurrency.mm} | 2 +- Sources/Sentry/SentryProfiler.mm | 2 +- Sources/Sentry/SentryTracer.m | 2 +- 5 files changed, 12 insertions(+), 12 deletions(-) rename Sources/Sentry/{include/SentryTracerConcurrency.h => Profiling/SentryProfiledTracerConcurrency.h} (100%) rename Sources/Sentry/{SentryTracerConcurrency.mm => Profiling/SentryProfiledTracerConcurrency.mm} (98%) diff --git a/Sentry.xcodeproj/project.pbxproj b/Sentry.xcodeproj/project.pbxproj index 74bf0d534ab..4f3083ca2a5 100644 --- a/Sentry.xcodeproj/project.pbxproj +++ b/Sentry.xcodeproj/project.pbxproj @@ -652,8 +652,8 @@ 84AC61D729F75A98009EEF61 /* SentryDispatchFactory.m in Sources */ = {isa = PBXBuildFile; fileRef = 84AC61D529F75A98009EEF61 /* SentryDispatchFactory.m */; }; 84AC61D929F7643B009EEF61 /* TestDispatchFactory.swift in Sources */ = {isa = PBXBuildFile; fileRef = 84AC61D829F7643B009EEF61 /* TestDispatchFactory.swift */; }; 84AC61DB29F7654A009EEF61 /* TestDispatchSourceWrapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 84AC61DA29F7654A009EEF61 /* TestDispatchSourceWrapper.swift */; }; - 84AF45A629A7FFA500FBB177 /* SentryTracerConcurrency.h in Headers */ = {isa = PBXBuildFile; fileRef = 84AF45A429A7FFA500FBB177 /* SentryTracerConcurrency.h */; }; - 84AF45A729A7FFA500FBB177 /* SentryTracerConcurrency.mm in Sources */ = {isa = PBXBuildFile; fileRef = 84AF45A529A7FFA500FBB177 /* SentryTracerConcurrency.mm */; }; + 84AF45A629A7FFA500FBB177 /* SentryProfiledTracerConcurrency.h in Headers */ = {isa = PBXBuildFile; fileRef = 84AF45A429A7FFA500FBB177 /* SentryProfiledTracerConcurrency.h */; }; + 84AF45A729A7FFA500FBB177 /* SentryProfiledTracerConcurrency.mm in Sources */ = {isa = PBXBuildFile; fileRef = 84AF45A529A7FFA500FBB177 /* SentryProfiledTracerConcurrency.mm */; }; 84B7FA3529B285FC00AD93B1 /* Sentry.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 63AA759B1EB8AEF500D153DE /* Sentry.framework */; }; 84B7FA3629B285FF00AD93B1 /* SentryPrivate.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = D81A3488291D0AC0005A27A9 /* SentryPrivate.framework */; }; 84B7FA3C29B2876F00AD93B1 /* TestConstants.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7BAF3DD6243DD4A1008A5414 /* TestConstants.swift */; }; @@ -1594,8 +1594,8 @@ 84AC61D529F75A98009EEF61 /* SentryDispatchFactory.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryDispatchFactory.m; sourceTree = ""; }; 84AC61D829F7643B009EEF61 /* TestDispatchFactory.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestDispatchFactory.swift; sourceTree = ""; }; 84AC61DA29F7654A009EEF61 /* TestDispatchSourceWrapper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestDispatchSourceWrapper.swift; sourceTree = ""; }; - 84AF45A429A7FFA500FBB177 /* SentryTracerConcurrency.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryTracerConcurrency.h; path = include/SentryTracerConcurrency.h; sourceTree = ""; }; - 84AF45A529A7FFA500FBB177 /* SentryTracerConcurrency.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = SentryTracerConcurrency.mm; sourceTree = ""; }; + 84AF45A429A7FFA500FBB177 /* SentryProfiledTracerConcurrency.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = SentryProfiledTracerConcurrency.h; sourceTree = ""; }; + 84AF45A529A7FFA500FBB177 /* SentryProfiledTracerConcurrency.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = SentryProfiledTracerConcurrency.mm; sourceTree = ""; }; 84B7FA3B29B2866200AD93B1 /* SentryTestUtils-ObjC-BridgingHeader.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = "SentryTestUtils-ObjC-BridgingHeader.h"; sourceTree = ""; }; 84B7FA4729B2995A00AD93B1 /* DeploymentTargets.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = DeploymentTargets.xcconfig; sourceTree = ""; }; 84C47B2B2A09239100DAEB8A /* .codecov.yml */ = {isa = PBXFileReference; lastKnownFileType = text.yaml; path = .codecov.yml; sourceTree = ""; }; @@ -3113,12 +3113,14 @@ 03F84D1B27DD414C008FE43F /* SentryMachLogging.hpp */, 844EDD6B2949387000C86F34 /* SentryMetricProfiler.h */, 8454CF8B293EAF9A006AC140 /* SentryMetricProfiler.mm */, + 84AF45A429A7FFA500FBB177 /* SentryProfiledTracerConcurrency.h */, + 84AF45A529A7FFA500FBB177 /* SentryProfiledTracerConcurrency.mm */, 03F84D1127DD414C008FE43F /* SentryProfiler.h */, 03F84D2B27DD4191008FE43F /* SentryProfiler.mm */, 84A888FC28D9B11700C51DFD /* SentryProfiler+Private.h */, 0354A22A2A134D9C003C3A04 /* SentryProfilerState.h */, - 84281C642A57D36100EE88F2 /* SentryProfilerState+ObjCpp.h */, 84281C422A578E5600EE88F2 /* SentryProfilerState.mm */, + 84281C642A57D36100EE88F2 /* SentryProfilerState+ObjCpp.h */, 0356A56E288B4612008BF593 /* SentryProfilesSampler.h */, 0356A56F288B4612008BF593 /* SentryProfilesSampler.m */, 84354E0F29BF944900CDBB8B /* SentryProfileTimeseries.h */, @@ -3251,8 +3253,6 @@ 8E4E7C8125DAB2A5006AB9E2 /* SentryTracer.m */, D8B088B429C9E3FF00213258 /* SentryTracerConfiguration.h */, D8B088B529C9E3FF00213258 /* SentryTracerConfiguration.m */, - 84AF45A429A7FFA500FBB177 /* SentryTracerConcurrency.h */, - 84AF45A529A7FFA500FBB177 /* SentryTracerConcurrency.mm */, 8E8C57A525EEFC42001CEEFA /* SentryTracesSampler.h */, 8E8C57A025EEFC07001CEEFA /* SentryTracesSampler.m */, 8E4A037725F6F52100000D77 /* SentrySampleDecision.h */, @@ -3511,7 +3511,7 @@ 03F84D1E27DD414C008FE43F /* SentryBacktrace.hpp in Headers */, 63AA76991EB9C1C200D153DE /* SentryDefines.h in Headers */, D86B6835294348A400B8B1FC /* SentryAttachment+Private.h in Headers */, - 84AF45A629A7FFA500FBB177 /* SentryTracerConcurrency.h in Headers */, + 84AF45A629A7FFA500FBB177 /* SentryProfiledTracerConcurrency.h in Headers */, 7B2A70DB27D607CF008B0D15 /* SentryThreadWrapper.h in Headers */, 8EAE980B261E9F530073B6B3 /* SentryPerformanceTracker.h in Headers */, 63FE718520DA4C1100CDBAE8 /* SentryCrashC.h in Headers */, @@ -4112,7 +4112,7 @@ 63FE70AD20DA4C1000CDBAE8 /* SentryCrashCString.m in Sources */, 6304360B1EC0595B00C4D3FA /* NSData+SentryCompression.m in Sources */, 639889B81EDECFA800EA7442 /* SentryBreadcrumbTracker.m in Sources */, - 84AF45A729A7FFA500FBB177 /* SentryTracerConcurrency.mm in Sources */, + 84AF45A729A7FFA500FBB177 /* SentryProfiledTracerConcurrency.mm in Sources */, 7DC8310C2398283C0043DD9A /* SentryCrashIntegration.m in Sources */, 03F84D3227DD4191008FE43F /* SentryProfiler.mm in Sources */, 635B3F391EBC6E2500A6176D /* SentryAsynchronousOperation.m in Sources */, diff --git a/Sources/Sentry/include/SentryTracerConcurrency.h b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.h similarity index 100% rename from Sources/Sentry/include/SentryTracerConcurrency.h rename to Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.h diff --git a/Sources/Sentry/SentryTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm similarity index 98% rename from Sources/Sentry/SentryTracerConcurrency.mm rename to Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index d1981166204..d55a76e9f8b 100644 --- a/Sources/Sentry/SentryTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -1,4 +1,4 @@ -#import "SentryTracerConcurrency.h" +#import "SentryProfiledTracerConcurrency.h" #if SENTRY_TARGET_PROFILING_SUPPORTED diff --git a/Sources/Sentry/SentryProfiler.mm b/Sources/Sentry/SentryProfiler.mm index 0af3572801e..c0919c7f426 100644 --- a/Sources/Sentry/SentryProfiler.mm +++ b/Sources/Sentry/SentryProfiler.mm @@ -24,6 +24,7 @@ # import "SentryNSProcessInfoWrapper.h" # import "SentryNSTimerFactory.h" # import "SentryProfileTimeseries.h" +# import "SentryProfiledTracerConcurrency.h" # import "SentryProfilerState+ObjCpp.h" # import "SentrySample.h" # import "SentrySamplingProfiler.hpp" @@ -35,7 +36,6 @@ # import "SentryThreadWrapper.h" # import "SentryTime.h" # import "SentryTracer.h" -# import "SentryTracerConcurrency.h" # import "SentryTransaction.h" # import "SentryTransactionContext+Private.h" diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m index b65c838232e..67390c596c6 100644 --- a/Sources/Sentry/SentryTracer.m +++ b/Sources/Sentry/SentryTracer.m @@ -12,6 +12,7 @@ #import "SentryLog.h" #import "SentryNSTimerFactory.h" #import "SentryNoOpSpan.h" +#import "SentryProfiledTracerConcurrency.h" #import "SentryProfiler.h" #import "SentryProfilesSampler.h" #import "SentryProfilingConditionals.h" @@ -25,7 +26,6 @@ #import "SentryTime.h" #import "SentryTraceContext.h" #import "SentryTraceOrigins.h" -#import "SentryTracerConcurrency.h" #import "SentryTransaction.h" #import "SentryTransactionContext.h" #import "SentryUIViewControllerPerformanceTracker.h" From 352df6a91acf0f1ce604de410f6b53d22bdca359 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Fri, 14 Jul 2023 15:34:53 -0800 Subject: [PATCH 02/15] clean up profilers for discarded transactions --- .../SentryProfiledTracerConcurrency.h | 6 ++++ .../SentryProfiledTracerConcurrency.mm | 31 +++++++++++++++++++ Sources/Sentry/SentryTracer.m | 15 +++++++++ 3 files changed, 52 insertions(+) diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.h b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.h index 6c0b60f169e..4d8c04b668b 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.h +++ b/Sources/Sentry/Profiling/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 diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index d55a76e9f8b..069d9843721 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -66,6 +66,37 @@ _gTracersToProfilers[tracerKey] = profiler; } +void +discardProfilerForTracer(SentryTracer *tracer) +{ + std::lock_guard 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 (!SENTRY_CASSERT_RETURN(profiler != nil, + @"Expected a profiler to be associated with tracer id %@.", tracerKey)) { + return; + } + + const auto profilerKey = profiler.profileId.sentryIdString; + + [_gTracersToProfilers removeObjectForKey:tracerKey]; + [_gProfilersToTracers[profilerKey] removeObject:tracer]; + if ([_gProfilersToTracers[profilerKey] count] == 0) { + [_gProfilersToTracers removeObjectForKey:profilerKey]; + } + +# if SENTRY_HAS_UIKIT + if (_gProfilersToTracers.count == 0) { + [SentryDependencyContainer.sharedInstance.framesTracker resetProfilingTimestamps]; + } +# endif // SENTRY_HAS_UIKIT +} + SentryProfiler *_Nullable profilerForFinishedTracer(SentryTracer *tracer) { std::lock_guard l(_gStateLock); diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m index 67390c596c6..04314bb8cc7 100644 --- a/Sources/Sentry/SentryTracer.m +++ b/Sources/Sentry/SentryTracer.m @@ -425,10 +425,16 @@ - (void)finishInternal { [self cancelDeadlineTimer]; if (self.isFinished) { +#if SENTRY_TARGET_PROFILING_SUPPORTED + discardProfilerForTracer(self); +#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } @synchronized(self) { if (self.isFinished) { +#if SENTRY_TARGET_PROFILING_SUPPORTED + discardProfilerForTracer(self); +#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } // Keep existing status of auto generated transactions if set by the user. @@ -462,10 +468,16 @@ - (void)finishInternal SENTRY_LOG_INFO(@"Auto generated transaction exceeded the max duration of %f seconds. Not " @"capturing transaction.", SENTRY_AUTO_TRANSACTION_MAX_DURATION); +#if SENTRY_TARGET_PROFILING_SUPPORTED + discardProfilerForTracer(self); +#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } if (_hub == nil) { +#if SENTRY_TARGET_PROFILING_SUPPORTED + discardProfilerForTracer(self); +#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } @@ -479,6 +491,9 @@ - (void)finishInternal if (_configuration.idleTimeout > 0.0 && _children.count == 0) { SENTRY_LOG_DEBUG(@"Was waiting for timeout for UI event trace but it had no children, " @"will not keep transaction."); +#if SENTRY_TARGET_PROFILING_SUPPORTED + discardProfilerForTracer(self); +#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } From 0f5d1635f586e563d37175ce361e5aabfa5bf6f5 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Fri, 14 Jul 2023 17:06:03 -0800 Subject: [PATCH 03/15] also stop a running profiler --- Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index 069d9843721..70ec1a81271 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -88,6 +88,9 @@ [_gProfilersToTracers[profilerKey] removeObject:tracer]; if ([_gProfilersToTracers[profilerKey] count] == 0) { [_gProfilersToTracers removeObjectForKey:profilerKey]; + if ([profiler isRunning]) { + [profiler stopForReason:SentryProfilerTruncationReasonNormal]; + } } # if SENTRY_HAS_UIKIT From 1467d2062a91a041661b0dff95f76a1537cbd9fc Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Mon, 17 Jul 2023 15:29:22 -0800 Subject: [PATCH 04/15] only store the count of tracers in bookkeeping, not a set of references --- .../SentryProfiledTracerConcurrency.mm | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index 70ec1a81271..44a41093f48 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -16,14 +16,14 @@ # 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 *> *_gProfilersToTracers; + /* number of in-flight tracers */ NSNumber *> *_gProfilersToTracers; /** provided for fast access to a profiler given a tracer */ static NSMutableDictionary @@ -48,21 +48,14 @@ if (_gProfilersToTracers == nil) { _gProfilersToTracers = [NSMutableDictionary *> dictionaryWithObject:[NSMutableSet setWithObject:tracer] - forKey:profilerKey]; + /* number of in-flight tracers */ NSNumber *> + dictionary]; _gTracersToProfilers = [NSMutableDictionary - dictionaryWithObject:profiler - forKey:tracerKey]; - return; - } - - if (_gProfilersToTracers[profilerKey] == nil) { - _gProfilersToTracers[profilerKey] = [NSMutableSet setWithObject:tracer]; - } else { - [_gProfilersToTracers[profilerKey] addObject:tracer]; + dictionary]; } + _gProfilersToTracers[profilerKey] = @(_gProfilersToTracers[profilerKey].unsignedIntValue + 1); _gTracersToProfilers[tracerKey] = profiler; } @@ -85,8 +78,8 @@ const auto profilerKey = profiler.profileId.sentryIdString; [_gTracersToProfilers removeObjectForKey:tracerKey]; - [_gProfilersToTracers[profilerKey] removeObject:tracer]; - if ([_gProfilersToTracers[profilerKey] count] == 0) { + _gProfilersToTracers[profilerKey] = @(_gProfilersToTracers[profilerKey].unsignedIntValue - 1); + if ([_gProfilersToTracers[profilerKey] unsignedIntValue] == 0) { [_gProfilersToTracers removeObjectForKey:profilerKey]; if ([profiler isRunning]) { [profiler stopForReason:SentryProfilerTruncationReasonNormal]; @@ -118,8 +111,8 @@ const auto profilerKey = profiler.profileId.sentryIdString; [_gTracersToProfilers removeObjectForKey:tracerKey]; - [_gProfilersToTracers[profilerKey] removeObject:tracer]; - if ([_gProfilersToTracers[profilerKey] count] == 0) { + _gProfilersToTracers[profilerKey] = @(_gProfilersToTracers[profilerKey].unsignedIntValue - 1); + if ([_gProfilersToTracers[profilerKey] unsignedIntValue] == 0) { [_gProfilersToTracers removeObjectForKey:profilerKey]; if ([profiler isRunning]) { [profiler stopForReason:SentryProfilerTruncationReasonNormal]; From ea7a894c7abf6a2bd2bc9047afaa5d72983c6016 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Tue, 18 Jul 2023 12:23:21 -0800 Subject: [PATCH 05/15] just call discard profiler from SentryTracer.dealloc --- .../SentryProfiledTracerConcurrency.mm | 45 ++++++++++--------- Sources/Sentry/SentryTracer.m | 22 +++------ 2 files changed, 32 insertions(+), 35 deletions(-) diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index 44a41093f48..bf9ff51d308 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -29,6 +29,29 @@ static NSMutableDictionary *_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 @@ -75,16 +98,7 @@ return; } - 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]; - } - } + _unsafe_cleanUpProfiler(profiler, tracerKey); # if SENTRY_HAS_UIKIT if (_gProfilersToTracers.count == 0) { @@ -108,16 +122,7 @@ return nil; } - 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]; - } - } + _unsafe_cleanUpProfiler(profiler, tracerKey); # if SENTRY_HAS_UIKIT profiler._screenFrameData = diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m index e05de120b03..4f942a1083e 100644 --- a/Sources/Sentry/SentryTracer.m +++ b/Sources/Sentry/SentryTracer.m @@ -173,6 +173,13 @@ - (instancetype)initWithTransactionContext:(SentryTransactionContext *)transacti return self; } +- (void)dealloc +{ +#if SENTRY_TARGET_PROFILING_SUPPORTED + discardProfilerForTracer(self); +#endif // SENTRY_TARGET_PROFILING_SUPPORTED +} + - (nullable SentryTracer *)tracer { return self; @@ -439,16 +446,10 @@ - (void)finishInternal { [self cancelDeadlineTimer]; if (self.isFinished) { -#if SENTRY_TARGET_PROFILING_SUPPORTED - discardProfilerForTracer(self); -#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } @synchronized(self) { if (self.isFinished) { -#if SENTRY_TARGET_PROFILING_SUPPORTED - discardProfilerForTracer(self); -#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } // Keep existing status of auto generated transactions if set by the user. @@ -484,16 +485,10 @@ - (void)finishInternal SENTRY_LOG_INFO(@"Auto generated transaction exceeded the max duration of %f seconds. Not " @"capturing transaction.", SENTRY_AUTO_TRANSACTION_MAX_DURATION); -#if SENTRY_TARGET_PROFILING_SUPPORTED - discardProfilerForTracer(self); -#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } if (_hub == nil) { -#if SENTRY_TARGET_PROFILING_SUPPORTED - discardProfilerForTracer(self); -#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } @@ -507,9 +502,6 @@ - (void)finishInternal if (_configuration.idleTimeout > 0.0 && _children.count == 0) { SENTRY_LOG_DEBUG(@"Was waiting for timeout for UI event trace but it had no children, " @"will not keep transaction."); -#if SENTRY_TARGET_PROFILING_SUPPORTED - discardProfilerForTracer(self); -#endif // SENTRY_TARGET_PROFILING_SUPPORTED return; } From d53dce1356f45589e57aabf6714ee197cf6fbbb0 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Tue, 18 Jul 2023 12:23:33 -0800 Subject: [PATCH 06/15] stub tests to write --- .../SentryProfilerSwiftTests.swift | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift index 1023a80bea3..4dfa9b32df5 100644 --- a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift +++ b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift @@ -401,6 +401,38 @@ class SentryProfilerSwiftTests: XCTestCase { options.profilesSampler = { _ in return -0.01 } } } + + /// based on ``SentryTracerTests.testFinish_WithoutHub_DoesntCaptureTransaction`` + func testProfilerCleanedUpAfterTransactionDiscarded_NoHub() { + + } + + /// based on ``SentryTracerTests.testFinish_WaitForAllChildren_ExceedsMaxDuration_NoTransactionCaptured`` + func testProfilerCleanedUpAfterTransactionDiscarded_ExceedsMaxDuration() { + + } + + /// based on ``SentryTracerTests.testFinish_IdleTimeout_ExceedsMaxDuration_NoTransactionCaptured`` + func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_ExceedsMaxDuration() { + + } + + /// based on ``SentryTracerTests.testIdleTimeout_NoChildren_TransactionNotCaptured`` + func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_NoChildren() { + + } + + /// based on ``SentryTracerTests.testIdleTransaction_CreatingDispatchBlockFails_NoTransactionCaptured`` + func testProfilerCleanedUpAfterTransactionDiscarded_IdleTransaction_CreatingDispatchBlockFails() { + + } + +#if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst) + /// based on ``SentryTracerTests.testFinish_WaitForAllChildren_StartTimeModified_NoTransactionCaptured`` + func testProfilerCleanedUpAfterTransactionDiscarded_WaitForAllChildren_StartTimeModified() { + + } +#endif // os(iOS) || os(tvOS) || targetEnvironment(macCatalyst) } private extension SentryProfilerSwiftTests { From 45eed31a5ba693e3154c18fdbfe2874701a84122 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Tue, 18 Jul 2023 14:23:47 -0800 Subject: [PATCH 07/15] fixup! stub tests to write --- Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift index 4dfa9b32df5..503dec762d9 100644 --- a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift +++ b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift @@ -403,8 +403,13 @@ class SentryProfilerSwiftTests: XCTestCase { } /// based on ``SentryTracerTests.testFinish_WithoutHub_DoesntCaptureTransaction`` - func testProfilerCleanedUpAfterTransactionDiscarded_NoHub() { + func testProfilerCleanedUpAfterTransactionDiscarded_NoHub() throws { + let sut = SentryTracer(transactionContext: TransactionContext(name: transactionName, operation: transactionOperation), hub: nil) + sut.finish() + let envelope = try XCTUnwrap(self.fixture.client?.captureEventWithScopeInvocations.last) + XCTAssertEqual(1, envelope.additionalEnvelopeItems.count) + let profileItem = try XCTUnwrap(envelope.additionalEnvelopeItems.first) } /// based on ``SentryTracerTests.testFinish_WaitForAllChildren_ExceedsMaxDuration_NoTransactionCaptured`` From a36628ee4fe29f70bac3f085065d78dd06cabcd0 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Tue, 18 Jul 2023 15:26:40 -0800 Subject: [PATCH 08/15] fixup! Merge branch 'main' into armcknight/fix/profiling-transaction-bookkeeping-cleanup --- Sentry.xcodeproj/project.pbxproj | 8 -------- SentryTestUtils/ClearTestState.swift | 2 +- .../Profiling/SentryProfiledTracerConcurrency.mm | 7 +++++++ Sources/Sentry/SentryProfiler.mm | 13 +++++++++++++ Sources/Sentry/SentryTracer.m | 14 +++----------- .../include/SentryProfiledTracerConcurrency.h | 1 + .../SentryProfilerSwiftTests.swift | 9 ++++----- Tests/SentryTests/SentryProfiler+Test.h | 4 ++++ Tests/SentryTests/SentryTests-Bridging-Header.h | 15 ++++++++++----- Tests/SentryTests/Transaction/SentryTracer+Test.h | 5 ----- 10 files changed, 43 insertions(+), 35 deletions(-) diff --git a/Sentry.xcodeproj/project.pbxproj b/Sentry.xcodeproj/project.pbxproj index 75e872cc08f..675a1babce7 100644 --- a/Sentry.xcodeproj/project.pbxproj +++ b/Sentry.xcodeproj/project.pbxproj @@ -1827,13 +1827,6 @@ /* End PBXFrameworksBuildPhase section */ /* Begin PBXGroup section */ - 035E73C627D5661A005EEB11 /* Profiling */ = { - isa = PBXGroup; - children = ( - ); - path = Profiling; - sourceTree = ""; - }; 0A9BF4E028A114690068D266 /* ViewHierarchy */ = { isa = PBXGroup; children = ( @@ -2255,7 +2248,6 @@ 7BD7299B24654CD500EA3610 /* Helper */, 7B944FA924697E9700A10721 /* Integrations */, 7BBD18AF24517E5D00427C76 /* Networking */, - 035E73C627D5661A005EEB11 /* Profiling */, 7B3D0474249A3D5800E106B6 /* Protocol */, 63FE71D220DA66C500CDBAE8 /* SentryCrash */, 7B944FAC2469B41600A10721 /* State */, diff --git a/SentryTestUtils/ClearTestState.swift b/SentryTestUtils/ClearTestState.swift index ddb2c3ea397..1fdfdf53796 100644 --- a/SentryTestUtils/ClearTestState.swift +++ b/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) diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index bf9ff51d308..ab89af75bbc 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -143,6 +143,13 @@ [_gTracersToProfilers removeAllObjects]; [_gProfilersToTracers removeAllObjects]; } + +NSUInteger +currentProfiledTracers() +{ + std::lock_guard l(_gStateLock); + return [_gTracersToProfilers count]; +} # endif // defined(TEST) || defined(TESTCI) #endif // SENTRY_TARGET_PROFILING_SUPPORTED diff --git a/Sources/Sentry/SentryProfiler.mm b/Sources/Sentry/SentryProfiler.mm index 8045af49c3c..c7fb2222f1b 100644 --- a/Sources/Sentry/SentryProfiler.mm +++ b/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 diff --git a/Sources/Sentry/SentryTracer.m b/Sources/Sentry/SentryTracer.m index 2826dffc93c..880e194802d 100644 --- a/Sources/Sentry/SentryTracer.m +++ b/Sources/Sentry/SentryTracer.m @@ -175,7 +175,9 @@ - (instancetype)initWithTransactionContext:(SentryTransactionContext *)transacti - (void)dealloc { #if SENTRY_TARGET_PROFILING_SUPPORTED - discardProfilerForTracer(self); + if (self.isProfiling) { + discardProfilerForTracer(self); + } #endif // SENTRY_TARGET_PROFILING_SUPPORTED } @@ -838,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 diff --git a/Sources/Sentry/include/SentryProfiledTracerConcurrency.h b/Sources/Sentry/include/SentryProfiledTracerConcurrency.h index 4d8c04b668b..058a977e02e 100644 --- a/Sources/Sentry/include/SentryProfiledTracerConcurrency.h +++ b/Sources/Sentry/include/SentryProfiledTracerConcurrency.h @@ -33,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 diff --git a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift index 503dec762d9..cb97a3947a6 100644 --- a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift +++ b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift @@ -404,12 +404,11 @@ class SentryProfilerSwiftTests: XCTestCase { /// based on ``SentryTracerTests.testFinish_WithoutHub_DoesntCaptureTransaction`` func testProfilerCleanedUpAfterTransactionDiscarded_NoHub() throws { - let sut = SentryTracer(transactionContext: TransactionContext(name: transactionName, operation: transactionOperation), hub: nil) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + let sut = SentryTracer(transactionContext: TransactionContext(name: fixture.transactionName, operation: fixture.transactionOperation), hub: nil) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) sut.finish() - - let envelope = try XCTUnwrap(self.fixture.client?.captureEventWithScopeInvocations.last) - XCTAssertEqual(1, envelope.additionalEnvelopeItems.count) - let profileItem = try XCTUnwrap(envelope.additionalEnvelopeItems.first) + XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } /// based on ``SentryTracerTests.testFinish_WaitForAllChildren_ExceedsMaxDuration_NoTransactionCaptured`` diff --git a/Tests/SentryTests/SentryProfiler+Test.h b/Tests/SentryTests/SentryProfiler+Test.h index 83a7655ed1b..1d94abfc7e1 100644 --- a/Tests/SentryTests/SentryProfiler+Test.h +++ b/Tests/SentryTests/SentryProfiler+Test.h @@ -10,6 +10,10 @@ SentryProfiler () + (SentryProfiler *)getCurrentProfiler; ++ (void)resetConcurrencyTracking; + ++ (NSUInteger)currentProfiledTracers; + @end NS_ASSUME_NONNULL_END diff --git a/Tests/SentryTests/SentryTests-Bridging-Header.h b/Tests/SentryTests/SentryTests-Bridging-Header.h index d0c129450d5..6f6a6679294 100644 --- a/Tests/SentryTests/SentryTests-Bridging-Header.h +++ b/Tests/SentryTests/SentryTests-Bridging-Header.h @@ -1,7 +1,9 @@ #if !TARGET_OS_WATCH # import "SentryReachability.h" #endif // !TARGET_OS_WATCH + #import "SentryDefines.h" +#import "SentryProfilingConditionals.h" #if SENTRY_HAS_METRIC_KIT # import "SentryMetricKitIntegration.h" @@ -19,6 +21,14 @@ # import "SentryUIViewControllerSwizzling.h" #endif // SENTRY_HAS_UIKIT +#if SENTRY_TARGET_PROFILING_SUPPORTED +# import "SentryMetricProfiler.h" +# import "SentryProfiler+Private.h" +# import "SentryProfiler+Test.h" +# import "SentryProfilerMocksSwiftCompatible.h" +# import "SentryProfilerState.h" +#endif // SENTRY_TARGET_PROFILING_SUPPORTED + #import "NSData+Sentry.h" #import "NSData+SentryCompression.h" #import "NSDate+SentryExtras.h" @@ -118,7 +128,6 @@ #import "SentryMechanism.h" #import "SentryMechanismMeta.h" #import "SentryMeta.h" -#import "SentryMetricProfiler.h" #import "SentryMigrateSessionInit.h" #import "SentryNSDataTracker.h" #import "SentryNSError.h" @@ -137,10 +146,6 @@ #import "SentryPerformanceTracker.h" #import "SentryPerformanceTrackingIntegration.h" #import "SentryPredicateDescriptor.h" -#import "SentryProfiler+Private.h" -#import "SentryProfiler+Test.h" -#import "SentryProfilerMocksSwiftCompatible.h" -#import "SentryProfilerState.h" #import "SentryQueueableRequestManager.h" #import "SentryRandom.h" #import "SentryRateLimitParser.h" diff --git a/Tests/SentryTests/Transaction/SentryTracer+Test.h b/Tests/SentryTests/Transaction/SentryTracer+Test.h index 3444b3f8b46..169d2777c09 100644 --- a/Tests/SentryTests/Transaction/SentryTracer+Test.h +++ b/Tests/SentryTests/Transaction/SentryTracer+Test.h @@ -1,4 +1,3 @@ -#import "SentryProfilingConditionals.h" #import "SentryTracer.h" NS_ASSUME_NONNULL_BEGIN @@ -10,10 +9,6 @@ SentryTracer (Test) - (void)updateStartTime:(NSDate *)startTime; -#if SENTRY_TARGET_PROFILING_SUPPORTED && (defined(TEST) || defined(TESTCI)) -+ (void)resetConcurrencyTracking; -#endif // SENTRY_TARGET_PROFILING_SUPPORTED && (defined(TEST) || defined(TESTCI)) - @end NS_ASSUME_NONNULL_END From 5c2add3efc31155585f14de8a862d0a1d7b9e62a Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Tue, 18 Jul 2023 16:23:12 -0800 Subject: [PATCH 09/15] fixup! fixup! Merge branch 'main' into armcknight/fix/profiling-transaction-bookkeeping-cleanup --- .../SentryProfilerSwiftTests.swift | 73 +++++++++++++++---- 1 file changed, 59 insertions(+), 14 deletions(-) diff --git a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift index cb97a3947a6..ca573dfeec5 100644 --- a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift +++ b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift @@ -30,6 +30,7 @@ class SentryProfilerSwiftTests: XCTestCase { lazy var dispatchFactory = TestDispatchFactory() var metricTimerFactory: TestDispatchSourceWrapper? lazy var timeoutTimerFactory = TestSentryNSTimerFactory() + let dispatchQueueWrapper = TestSentryDispatchQueueWrapper() let currentDateProvider = TestCurrentDateProvider() @@ -63,11 +64,25 @@ class SentryProfilerSwiftTests: XCTestCase { } /// Advance the mock date provider, start a new transaction and return its handle. - func newTransaction(testingAppLaunchSpans: Bool = false) throws -> SentryTracer { - if testingAppLaunchSpans { - return try XCTUnwrap(hub.startTransaction(name: transactionName, operation: SentrySpanOperationUILoad) as? SentryTracer) + func newTransaction(testingAppLaunchSpans: Bool = false, automaticTransaction: Bool = false, idleTimeout: TimeInterval? = nil) throws -> SentryTracer { + let operation = testingAppLaunchSpans ? SentrySpanOperationUILoad : transactionOperation + + if automaticTransaction { + return hub.startTransaction( + with: TransactionContext(name: transactionName, operation: operation), + bindToScope: false, + customSamplingContext: [:], + configuration: SentryTracerConfiguration(block: { + if let idleTimeout = idleTimeout { + $0.idleTimeout = idleTimeout + } + $0.dispatchQueueWrapper = self.dispatchQueueWrapper + $0.waitForChildren = true + $0.timerFactory = self.timeoutTimerFactory + })) } - return try XCTUnwrap(hub.startTransaction(name: transactionName, operation: transactionOperation) as? SentryTracer) + + return try XCTUnwrap(hub.startTransaction(name: transactionName, operation: operation) as? SentryTracer) } // mocking @@ -412,29 +427,59 @@ class SentryProfilerSwiftTests: XCTestCase { } /// based on ``SentryTracerTests.testFinish_WaitForAllChildren_ExceedsMaxDuration_NoTransactionCaptured`` - func testProfilerCleanedUpAfterTransactionDiscarded_ExceedsMaxDuration() { - + func testProfilerCleanedUpAfterTransactionDiscarded_ExceedsMaxDuration() throws { + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + let sut = try fixture.newTransaction(automaticTransaction: true) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + sut.finish() + XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } /// based on ``SentryTracerTests.testFinish_IdleTimeout_ExceedsMaxDuration_NoTransactionCaptured`` - func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_ExceedsMaxDuration() { - + func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_ExceedsMaxDuration() throws { + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + let sut = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + sut.finish() + XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } /// based on ``SentryTracerTests.testIdleTimeout_NoChildren_TransactionNotCaptured`` - func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_NoChildren() { - + func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_NoChildren() throws { + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + let span = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + fixture.dispatchQueueWrapper.invokeLastDispatchAfter() + XCTAssert(span.isFinished) + XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } /// based on ``SentryTracerTests.testIdleTransaction_CreatingDispatchBlockFails_NoTransactionCaptured`` - func testProfilerCleanedUpAfterTransactionDiscarded_IdleTransaction_CreatingDispatchBlockFails() { - + func testProfilerCleanedUpAfterTransactionDiscarded_IdleTransaction_CreatingDispatchBlockFails() throws { + fixture.dispatchQueueWrapper.createDispatchBlockReturnsNULL = true + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + let span = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + span.finish() + XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } #if os(iOS) || os(tvOS) || targetEnvironment(macCatalyst) /// based on ``SentryTracerTests.testFinish_WaitForAllChildren_StartTimeModified_NoTransactionCaptured`` - func testProfilerCleanedUpAfterTransactionDiscarded_WaitForAllChildren_StartTimeModified() { - + func testProfilerCleanedUpAfterTransactionDiscarded_WaitForAllChildren_StartTimeModified() throws { + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + let appStartMeasurement = fixture.getAppStartMeasurement(type: .cold) + SentrySDK.setAppStartMeasurement(appStartMeasurement) + fixture.currentDateProvider.advance(by: 1) + let sut = try fixture.newTransaction(testingAppLaunchSpans: true, automaticTransaction: true) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 499) + sut.finish() + XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } #endif // os(iOS) || os(tvOS) || targetEnvironment(macCatalyst) } From e5092c047867f30319c6b84083c14e8a87dbad2a Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Tue, 18 Jul 2023 16:32:59 -0800 Subject: [PATCH 10/15] stronger checks --- .../SentryProfilerSwiftTests.swift | 70 +++++++++++++------ 1 file changed, 47 insertions(+), 23 deletions(-) diff --git a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift index ca573dfeec5..9a9433469fd 100644 --- a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift +++ b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift @@ -420,40 +420,56 @@ class SentryProfilerSwiftTests: XCTestCase { /// based on ``SentryTracerTests.testFinish_WithoutHub_DoesntCaptureTransaction`` func testProfilerCleanedUpAfterTransactionDiscarded_NoHub() throws { XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) - let sut = SentryTracer(transactionContext: TransactionContext(name: fixture.transactionName, operation: fixture.transactionOperation), hub: nil) + func performTransaction() { + let sut = SentryTracer(transactionContext: TransactionContext(name: fixture.transactionName, operation: fixture.transactionOperation), hub: nil) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + sut.finish() + } + performTransaction() XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) - sut.finish() XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } /// based on ``SentryTracerTests.testFinish_WaitForAllChildren_ExceedsMaxDuration_NoTransactionCaptured`` func testProfilerCleanedUpAfterTransactionDiscarded_ExceedsMaxDuration() throws { XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) - let sut = try fixture.newTransaction(automaticTransaction: true) - XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) - fixture.currentDateProvider.advance(by: 500) - sut.finish() + func performTransaction() throws { + let sut = try fixture.newTransaction(automaticTransaction: true) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + sut.finish() + } + try performTransaction() + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } /// based on ``SentryTracerTests.testFinish_IdleTimeout_ExceedsMaxDuration_NoTransactionCaptured`` func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_ExceedsMaxDuration() throws { XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) - let sut = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) - XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) - fixture.currentDateProvider.advance(by: 500) - sut.finish() + func performTransaction() throws { + let sut = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + sut.finish() + } + try performTransaction() + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } /// based on ``SentryTracerTests.testIdleTimeout_NoChildren_TransactionNotCaptured`` func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_NoChildren() throws { XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) - let span = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) - XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) - fixture.currentDateProvider.advance(by: 500) - fixture.dispatchQueueWrapper.invokeLastDispatchAfter() - XCTAssert(span.isFinished) + func performTransaction() throws { + let span = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + fixture.dispatchQueueWrapper.invokeLastDispatchAfter() + XCTAssert(span.isFinished) + } + try performTransaction() + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } @@ -461,10 +477,14 @@ class SentryProfilerSwiftTests: XCTestCase { func testProfilerCleanedUpAfterTransactionDiscarded_IdleTransaction_CreatingDispatchBlockFails() throws { fixture.dispatchQueueWrapper.createDispatchBlockReturnsNULL = true XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) - let span = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) - XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) - fixture.currentDateProvider.advance(by: 500) - span.finish() + func performTransaction() throws { + let span = try fixture.newTransaction(automaticTransaction: true, idleTimeout: 1) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 500) + span.finish() + } + try performTransaction() + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } @@ -475,10 +495,14 @@ class SentryProfilerSwiftTests: XCTestCase { let appStartMeasurement = fixture.getAppStartMeasurement(type: .cold) SentrySDK.setAppStartMeasurement(appStartMeasurement) fixture.currentDateProvider.advance(by: 1) - let sut = try fixture.newTransaction(testingAppLaunchSpans: true, automaticTransaction: true) - XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) - fixture.currentDateProvider.advance(by: 499) - sut.finish() + func performTransaction() throws { + let sut = try fixture.newTransaction(testingAppLaunchSpans: true, automaticTransaction: true) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + fixture.currentDateProvider.advance(by: 499) + sut.finish() + } + try performTransaction() + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } #endif // os(iOS) || os(tvOS) || targetEnvironment(macCatalyst) From e452f9888882f04f7a425a67749d54e790e4e489 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Wed, 19 Jul 2023 16:02:29 -0800 Subject: [PATCH 11/15] add more tests --- .../SentryProfilerSwiftTests.swift | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift index 9a9433469fd..30e025cfbcf 100644 --- a/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift +++ b/Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift @@ -258,10 +258,12 @@ class SentryProfilerSwiftTests: XCTestCase { func createConcurrentSpansWithMetrics() throws { XCTAssertFalse(SentryProfiler.isCurrentlyProfiling()) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) - for _ in 0 ..< numberOfTransactions { + for i in 0 ..< numberOfTransactions { let span = try fixture.newTransaction() XCTAssertTrue(SentryProfiler.isCurrentlyProfiling()) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(i + 1)) spans.append(span) fixture.currentDateProvider.advanceBy(nanoseconds: 100) } @@ -272,12 +274,14 @@ class SentryProfilerSwiftTests: XCTestCase { try fixture.gatherMockedMetrics(span: span) XCTAssertTrue(SentryProfiler.isCurrentlyProfiling()) span.finish() + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(numberOfTransactions - i - 1)) try self.assertValidProfileData() try self.assertMetricsPayload(metricsBatches: i + 1) } XCTAssertFalse(SentryProfiler.isCurrentlyProfiling()) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) } try createConcurrentSpansWithMetrics() @@ -444,6 +448,18 @@ class SentryProfilerSwiftTests: XCTestCase { XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) } + func testProfilerCleanedUpAfterInFlightTransactionDeallocated() throws { + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + func performTransaction() throws { + let sut = try fixture.newTransaction(automaticTransaction: true) + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(1)) + XCTAssertFalse(sut.isFinished) + } + try performTransaction() + XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) + XCTAssertEqual(self.fixture.client?.captureEventWithScopeInvocations.count, 0) + } + /// based on ``SentryTracerTests.testFinish_IdleTimeout_ExceedsMaxDuration_NoTransactionCaptured`` func testProfilerCleanedUpAfterTransactionDiscarded_IdleTimeout_ExceedsMaxDuration() throws { XCTAssertEqual(SentryProfiler.currentProfiledTracers(), UInt(0)) From 8c2ade9ce407c0bc6cd47c61d92c719741c41518 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Wed, 19 Jul 2023 16:05:55 -0800 Subject: [PATCH 12/15] fix tvos build --- SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h b/SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h index 743dc44fa6e..63ab8d9a5da 100644 --- a/SentryTestUtils/SentryTestUtils-ObjC-BridgingHeader.h +++ b/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" From 097e8345faa7b3e33c07d6b02b832448ba4d116f Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Thu, 20 Jul 2023 17:06:11 -0800 Subject: [PATCH 13/15] dont assert presence of profiler in discard; it may have already been finished normally by tracer --- Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index ab89af75bbc..3f5620659be 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -93,11 +93,6 @@ const auto tracerKey = tracer.traceId.sentryIdString; const auto profiler = _gTracersToProfilers[tracerKey]; - if (!SENTRY_CASSERT_RETURN(profiler != nil, - @"Expected a profiler to be associated with tracer id %@.", tracerKey)) { - return; - } - _unsafe_cleanUpProfiler(profiler, tracerKey); # if SENTRY_HAS_UIKIT From 95dcedbc84caba9479c9abd4c8a3ab95110da0c1 Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Thu, 20 Jul 2023 17:07:26 -0800 Subject: [PATCH 14/15] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54c36334dc9..1ac1b56a22e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Improvements - Reduced macOS SDK footprint by 2% (#3157) with similar changes for tvOS and watchOS (#3158, #3159, #3161) +- Reclaim memory used by profiler when transactions are discarded (#3154) ### Fixes From 0529a52dcfc6bf8fc04ed62b16229bcd8b684a4d Mon Sep 17 00:00:00 2001 From: Andrew McKnight Date: Fri, 21 Jul 2023 11:18:42 -0800 Subject: [PATCH 15/15] fix changelog and test failure --- CHANGELOG.md | 7 ++++++- .../Sentry/Profiling/SentryProfiledTracerConcurrency.mm | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 996019d3470..687532d9eb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,16 @@ # Changelog +## Unreleased + +### Fixes + +- Reclaim memory used by profiler when transactions are discarded (#3154) + ## 8.9.2 ### Improvements - Reduced macOS SDK footprint by 2% (#3157) with similar changes for tvOS and watchOS (#3158, #3159, #3161) -- Reclaim memory used by profiler when transactions are discarded (#3154) ### Fixes diff --git a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm index 3f5620659be..14b3f4d4daf 100644 --- a/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm +++ b/Sources/Sentry/Profiling/SentryProfiledTracerConcurrency.mm @@ -93,6 +93,10 @@ const auto tracerKey = tracer.traceId.sentryIdString; const auto profiler = _gTracersToProfilers[tracerKey]; + if (profiler == nil) { + return; + } + _unsafe_cleanUpProfiler(profiler, tracerKey); # if SENTRY_HAS_UIKIT