Automattic / Automattic-Tracks-iOS

Client library for tracking user events for later analysis
GNU General Public License v2.0
41 stars 12 forks source link

Optimized the logic that increments the upload retry count. #201

Closed diegoreymendez closed 2 years ago

diegoreymendez commented 2 years ago

This PR attempts to address performance issues we're seeing where calls to -[TracksEventService incrementRetryCountForEvents:] are blocking the main thread.

Here's an example of the first part of the sample file:

Call graph:
    2164 Thread_6960953: Main Thread   DispatchQueue_<multiple>
    + 2164 start  (in dyld) + 520  [0x1028dd0f4]
    +   2164 NSApplicationMain  (in AppKit) + 1064  [0x1b174f088]
    +     2164 -[NSApplication run]  (in AppKit) + 596  [0x1b177d9b4]
    +       2164 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]  (in AppKit) + 1332  [0x1b178b8a4]
    +         2164 _DPSNextEvent  (in AppKit) + 844  [0x1b178d000]
    +           2164 _BlockUntilNextEventMatchingListInModeWithFilter  (in HIToolbox) + 72  [0x1b77cea9c]
    +             2164 ReceiveNextEventCommon  (in HIToolbox) + 552  [0x1b77cecdc]
    +               2164 RunCurrentEventLoopInMode  (in HIToolbox) + 292  [0x1b77cef68]
    +                 2164 CFRunLoopRunSpecific  (in CoreFoundation) + 600  [0x1aec36734]
    +                   2164 __CFRunLoopRun  (in CoreFoundation) + 2540  [0x1aec378c0]
    +                     2164 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__  (in CoreFoundation) + 16  [0x1aec79f00]
    +                       2164 _dispatch_main_queue_callback_4CF  (in libdispatch.dylib) + 944  [0x1ae9bd0ac]
    +                         2164 _dispatch_client_callout  (in libdispatch.dylib) + 20  [0x1ae9aebac]
    +                           2164 _dispatch_call_block_and_release  (in libdispatch.dylib) + 32  [0x1ae9ace60]
    +                             2164 __33-[TracksService sendQueuedEvents]_block_invoke (in DOCore) (TracksService.m:0)
    +                               2164 -[TracksEventService incrementRetryCountForEvents:] (in DOCore) (TracksEventService.m:0)
    +                                 2164 -[TracksEventPersistenceService incrementRetryCountForEvents:] (in DOCore) (TracksEventPersistenceService.m:108)
    +                                   2164 -[NSManagedObjectContext performBlockAndWait:]  (in CoreData) + 280  [0x1b4cb0380]
    +                                     2164 _dispatch_lane_barrier_sync_invoke_and_complete  (in libdispatch.dylib) + 56  [0x1ae9bde00]
    +                                       2164 _dispatch_client_callout  (in libdispatch.dylib) + 20  [0x1ae9aebac]
    +                                         2164 developerSubmittedBlockToNSManagedObjectContextPerform  (in CoreData) + 156  [0x1b4cb047c]
    +                                           2164 __62-[TracksEventPersistenceService incrementRetryCountForEvents:]_block_invoke (in DOCore) (TracksEventPersistenceService.m:0)
    +                                             2163 -[TracksEventPersistenceService findTracksEventCoreDataWithUUID:] (in DOCore) (TracksEventPersistenceService.m:0)
    +                                             ! 1942 -[NSManagedObjectContext executeFetchRequest:error:]  (in CoreData) + 3224  [0x1b4cb303c]
    +                                             ! : 1691 -[NSComparisonPredicate evaluateWithObject:substitutionVariables:]  (in Foundation) + 216  [0x1afb12e84]
    +                                             ! : | 890 -[NSFunctionExpression expressionValueWithObject:context:]  (in Foundation) + 648  [0x1afb131f8]
    +                                             ! : | + 214 +[_NSPredicateUtilities _predicateEnforceRestrictionsOnTarget:forComponentName:]  (in Foundation) + 292  [0x1afd19abc]
    +                                             ! : | + ! 195 -[NSConstantDictionary objectForKey:]  (in CoreFoundation) + 316  [0x1aec258cc]
    +                                             ! : | + ! : 192 bsearch  (in libsystem_c.dylib) + 68  [0x1aea3944c]
    +                                             ! : | + ! : | 153 comparisonUsingOrderingForStringKeys  (in CoreFoundation) + 84  [0x1aed22364]

Changes:

  1. The logic that increments the retry count no longer tries to run synchronously. This was apparently locking the main thread in DayOne under certain circumstances. Additionally if you look at the previous code you'll notice running it synchronously wasn't really being used to serialize execution.
  2. The logic that queries the events we need to increase the retry count now runs in batches of 500 events at a time.
  3. I've added unit tests that should help validate the changes.
  4. Because implementing this code in Swift was infinitely easier, I opened a path to incrementally migrate the AutomatticTracksEvents module to Swift (AutomatticTracksEventsForSwift). Once all code has been migrated, simply removing the ObjC module and renaming the Swift module should do.

Testing:

Review the unit tests:

Reviewing the unit tests and trying to find flaws in them is important because they're automatically validating our expectations.

Testing in Day One:

  1. Import the Swift Package locally into DOCore (this should replace the remote dependency) or update the remote dependency to use this branch.
  2. Place a breakpoint in TrackEventPersistenceService.swift:25.
  3. Disconnect your device from internet.
  4. Run Day One, and make sure event logging is enabled.
  5. Once the breakpoint hits, debug the method being run and make sure it increases the retry count for the specified events correctly.
diegoreymendez commented 2 years ago

Ready for another look.

diegoreymendez commented 2 years ago

Ready for another look.

diegoreymendez commented 2 years ago

Hey @bjhomer if you have a moment this is ready for another look. Thanks!

diegoreymendez commented 2 years ago

Thank you!