mixpanel / mixpanel-swift

Official iOS (Swift) Tracking Library for Mixpanel Analytics
https://mixpanel.com
Apache License 2.0
434 stars 234 forks source link

Stop serialize data through NSKeyedArchiver #433

Closed evnik closed 2 years ago

evnik commented 3 years ago

Our app gets a lot of high energy consumption reports with backtraces similar to this:

#0 in __CFBasicHashAddValue ()
#1 in CFBasicHashAddValue ()
#2 in CFDictionaryAddValue ()
#3 in _flattenPlist ()
#4 in _flattenPlist ()
#5 in _flattenPlist ()
#6 in _flattenPlist ()
#7 in __CFBinaryPlistWriteOrPresize ()
#8 in -[NSKeyedArchiver finishEncoding] ()
#9 in -[NSKeyedArchiver encodedData] ()
#10 in +[NSKeyedArchiver archivedDataWithRootObject:requiringSecureCoding:error:] ()
#11 0x4e606e4 in specialized static Persistence.archiveToFile(_:object:token:) at ~/Projects/mixpanel-swift/Sources/Persistence.swift:164
#12 in closure #1 in static Persistence.archiveEvents(_:token:) ()
#13 in partial apply for thunk for @callee_guaranteed () -> () ()
#14 in thunk for @escaping @callee_guaranteed () -> () ()
#15 in _dispatch_client_callout ()
#16 in _dispatch_lane_barrier_sync_invoke_and_complete ()
#17 0x4e31c24 in closure #3 in closure #1 in MixpanelInstance.track(event:properties:) at ~/Projects/mixpanel-swift/Sources/MixpanelInstance.swift:1245
#18 0x4e66858 in closure #1 in ReadWriteLock.read(closure:) at ~/Projects/mixpanel-swift/Sources/ReadWriteLock.swift:19
#19 in partial apply for thunk for @callee_guaranteed () -> () ()
#20 in thunk for @escaping @callee_guaranteed () -> () ()
#21 in _dispatch_client_callout ()
#22 in _dispatch_sync_invoke_and_complete ()
#23 0x4e31954 in closure #1 in MixpanelInstance.track(event:properties:) at ~/Projects/mixpanel-swift/Sources/MixpanelInstance.swift:1244
#24 in thunk for @escaping @callee_guaranteed () -> () ()
#25 in _dispatch_call_block_and_release ()
#26 in _dispatch_client_callout ()
#27 in _dispatch_lane_serial_drain$VARIANT$armv81 ()
#28 in _dispatch_lane_invoke$VARIANT$armv81 ()
#29 in _dispatch_workloop_worker_thread ()
#30 in _pthread_wqthread ()

Screenshot from Xcode Organizer to make it more clear (this is a recent beta version, actually there are much more than 5 you can see here):

MixpanelBacktrace

From what I see, NSKeyedArchiver provides poor performance itself as well as comparing to other solutions, which means it also takes more energy to handle the data. In addition, Apple documentation says

Avoid using “$” as a prefix for your keys. The keyed archiver and unarchiver use keys prefixed with “$” for internal values. Although they test for and mangle user-defined keys that have a “$” prefix, this overhead makes archiving slower.

From what I understand, Mixpanel actively uses $ prefixed keys, which makes performance even worse and energy consumption higher.

Finally there are bunch of fixed and even active (like #431) crashes, seems to be caused by the NSKeyedArchiver

I believe these arguments should be enough to reconsider implementation of Mixpanel Persistence class.

zihejia commented 3 years ago

hi @evnik , thanks for bringing this up. We will consider it.

tealshift commented 3 years ago

It seems Mixpanel is too much to handle for Series 3 watches and older. My team is getting lots of crash reports from Series 3 and 2 with this signature Snapshot watchdog transgression. Exhausted CPU time allowance of 2.00 seconds and Mixpanel archiving almost always listed in the stack trace.

Thread 3:
0   CoreFoundation                  0x500147ca CFRetain + 18 (CFInternal.h:0)
1   CoreFoundation                  0x50048afc _CFArrayReplaceValues + 254 (CFArray.c:989)
2   CoreFoundation                  0x50048452 CFArrayAppendValue + 106 (CFArray.c:738)
3   CoreFoundation                  0x5005063e _flattenPlist + 188 (CFBinaryPList.c:494)
4   CoreFoundation                  0x50050758 _flattenPlist + 470 (CFBinaryPList.c:511)
5   CoreFoundation                  0x5005071c _flattenPlist + 410 (CFBinaryPList.c:502)
6   CoreFoundation                  0x50050758 _flattenPlist + 470 (CFBinaryPList.c:511)
7   CoreFoundation                  0x5005071c _flattenPlist + 410 (CFBinaryPList.c:502)
8   CoreFoundation                  0x500502ee __CFBinaryPlistWriteOrPresize + 192 (CFBinaryPList.c:571)
9   CoreFoundation                  0x50050cdc __CFBinaryPlistWriteToStreamWithOptions + 16 (CFBinaryPList.c:658)
10  Foundation                      0x50982e58 -[NSKeyedArchiver finishEncoding] + 422 (NSKeyedArchiver.m:797)
11  Foundation                      0x50983e54 -[NSKeyedArchiver encodedData] + 58 (NSKeyedArchiver.m:813)
12  Foundation                      0x509833ba +[NSKeyedArchiver archivedDataWithRootObject:requiringSecureCoding:error:] + 110 (NSKeyedArchiver.m:525)
13  [appname]                       0x006aca40 specialized static Persistence.archiveToFile(_:object:token:) + 600 (Persistence.swift:142)
14  [appname]                       0x006a9bee closure #1 in static Persistence.archiveEvents(_:token:) + 58
15  [appname]                       0x006a9b96 closure #1 in static Persistence.archivePeople(_:token:) + 22
16  [appname]                       0x006a93c2 partial apply for closure #1 in static Persistence.archivePeople(_:token:) + 22 (<compiler-generated>:0)
17  [appname]                       0x0069b0bc thunk for @callee_guaranteed () -> () + 12 (<compiler-generated>:0)
18  [appname]                       0x00695e5e thunk for @escaping @callee_guaranteed () -> () + 14 (<compiler-generated>:0)
19  libdispatch.dylib               0x4fc12e1e _dispatch_client_callout + 6 (object.m:495)
20  libdispatch.dylib               0x4fc1d49c _dispatch_lane_barrier_sync_invoke_and_complete + 38 (queue.c:996)
21  [appname]                       0x006a811c closure #4 in closure #1 in People.addPeopleRecordToQueueWithAction(_:properties:) + 330 (Persistence.swift:89)
22  [appname]                       0x006ad2d6 closure #1 in ReadWriteLock.read(closure:) + 12 (ReadWriteLock.swift:19)
23  [appname]                       0x0069b0bc thunk for @callee_guaranteed () -> () + 12 (<compiler-generated>:0)
24  [appname]                       0x00695e5e thunk for @escaping @callee_guaranteed () -> () + 14 (<compiler-generated>:0)
25  libdispatch.dylib               0x4fc12e1e _dispatch_client_callout + 6 (object.m:495)
26  libdispatch.dylib               0x4fc1d8f8 _dispatch_sync_invoke_and_complete + 38 (queue.c:996)
27  [appname]                       0x006a7e70 closure #1 in People.addPeopleRecordToQueueWithAction(_:properties:) + 5416 (ReadWriteLock.swift:18)
28  [appname]                       0x006a923e partial apply for closure #1 in People.addPeopleRecordToQueueWithAction(_:properties:) + 34 (<compiler-generated>:0)
29  [appname]                       0x00695e5e thunk for @escaping @callee_guaranteed () -> () + 14 (<compiler-generated>:0)
30  libdispatch.dylib               0x4fc11dc0 _dispatch_call_block_and_release + 10 (init.c:1408)
31  libdispatch.dylib               0x4fc12e1e _dispatch_client_callout + 6 (object.m:495)
32  libdispatch.dylib               0x4fc17d96 _dispatch_lane_serial_drain + 552 (inline_internal.h:2484)
33  libdispatch.dylib               0x4fc18630 _dispatch_lane_invoke + 320 (queue.c:3863)
34  libdispatch.dylib               0x4fc20334 _dispatch_workloop_worker_thread + 508 (queue.c:6445)
35  libsystem_pthread.dylib         0x4fde6cfc _pthread_wqthread + 204 (pthread.c:2351)

I've tried everything I could find to address the normal causes of the snapshot background task failing with this watchdog signature, but to no avail. I am currently disabling Mixpanel for older hardware to see if we get better results.

My team would really appreciate seeing this issue addressed. Thanks!

zihejia commented 3 years ago

hi @tealshift , we've been working on this, will switch to sqlite.

jaredmixpanel commented 3 years ago

@evnik @tealshift We have a new beta release that addresses this issue if you'd like to try it out here: https://github.com/mixpanel/mixpanel-swift/releases/tag/v3.0.0.beta.4 (Please do note the beta release does not include Messages & Experiments functionality)

jaredmixpanel commented 3 years ago

Instructions for installing the beta branch are here: https://github.com/mixpanel/mixpanel-swift/tree/3.0.0.beta#installation

foxware00 commented 3 years ago

@jaredmixpanel do you have a timeline you can share for 3.0.0 release? I'd love to move over the SQLite for event serialization but don't want to get bitten by shipping a beta.

jaredmixpanel commented 3 years ago

@foxware00 The intention is to keep the version without Messages & Experiments as "beta" until M&E is fully deprecated. We will move it out of beta after we get some feedback, but that won't be before we have deprecated M&E in January 2022.

foxware00 commented 3 years ago

@foxware00 The intention is to keep the version without Messages & Experiments as "beta" until M&E is fully deprecated. We will move it out of beta after we get some feedback, but that won't be before we have deprecated M&E in January 2022.

Thanks for the reply @jaredmixpanel. This SQLite change seems really cool and it'll be a shame to wait until January for all that goodness but I understand the timeline with M&E deprecations. Just don't want to be stuck in a SQL migration issue down the road.

Have you got a roadmap for the 3.0.0 release we can feedback on directly? I don't know if this issue is the best place.

jaredmixpanel commented 3 years ago

@foxware00 The 3.0 "beta" is just the current release, minus M&E code, plus SQLite. We're fully supporting it now. You can open any new issues about it here on Github.

foxware00 commented 3 years ago

@jaredmixpanel thanks for the response. I will have a play and see how it fits in!

RamblinWreck77 commented 3 years ago

@foxware00 We're using 3.0.0.beta in prod with minimal issues. (it resolves this issue).

zihejia commented 2 years ago

Marking it closed now. Since the 3.0.0 release is just around the corner(Jan 1, 2022) and we are encouraging everyone to try it out. We are fully supporting it!

Reminder: On Jan 1, 2022, Messages & Experiments will be removed from Mixapnel.