mixpanel / mixpanel-swift

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

archiveToFile questions #359

Closed RamblinWreck77 closed 4 years ago

RamblinWreck77 commented 4 years ago

Hello! Two questions:

1) In Persistence.swift, shouldn't archiveToFile be a static function and not a class function? AFAIK In general, dynamic dispatch is much slower... and looking at the code I don't see why you would ever want class instead of static (both being private). If my hunch about dynamic dispatch is true it would be interesting to do a benchmark with class private func and static private func and see which one performs better (with optimizations on!)

https://borgs.cybrilla.com/tils/global-vs-static-function-swift/

2) We still get energy-related terminations in our app from Persistence.swift, and every single one of them is in archiveToFile (which is why I was looking in the first place). I routinely see things like "90 seconds cpu time over 128 seconds (70% cpu average), exceeding limit of 50% cpu over 180 seconds"... in this particular case on an iPhone XS Max. There is nothing in our app capable of using 70% of an XS Max's CPU at 70% for 90 seconds. In addition, the stack traces returned always highlight the mixpanel thread and this particular function. Interestingly, most of the "heat" seems to be coming from within foundation which leads me to a few theories:

Caveat: Our users routinely use our app with no or poor network connections, so our event cache might be larger than normal

Theory A: Mixpanel is accumulating huge archives of events that haven't sent, and NSKeyedArchiver is working fine but the objects are so large CPU usage gets crazy.

Theory B: Something unique about the objects mixpanel is trying to archive is triggering some pathological use case in NSKeyedArchiver leading to crazy CPU consumption (and thus the energy/performance triggers)

Theory C: Something funky is happening because of archiveToFile's class func dynamic dispatch + lots of different queues (archiveCodelessQueue, archiveVariantQueue, archiveOptOutStatusQueue, etc) all trying to synchronously dispatch work to it. Going to static might fix this (and just in general), also moving archive ops from sync to async might fix it. The dispatchqueues outlined in lines 28-34 are all serial which will enforce a barrier between subsequent async dispatches, so as long as you don't need cross-queue time ordering to your archives.

If you do need cross-queue time ordering then perhaps a better way to do this would be to have 1 utility queue "com.mixpanel.archiveQueue" and async-dispatch all ops to it asynchronously. That way you have a 100% guarantee from the OS that async loads will be executed in the order they were broadcast.

If you'd like, I can get a PR up with both these changes and we can test it out :)

RamblinWreck77 commented 4 years ago

@zihejia PR #360 is up, if you get a chance to try this out I'd appreciate it :)

RamblinWreck77 commented 4 years ago

@zihejia just put #361 up as well, it includes the static function PR + the mono-archive-queue with async dispatch.

jserrafindster commented 4 years ago

@RamblinWreck77 which xcode are you using?

RamblinWreck77 commented 4 years ago

@jserrafindster 11.4

EDIT: 11.4.1 now

zihejia commented 4 years ago

hi @RamblinWreck77 , thanks so much for bring this up and providing the solution. I released a new version v2.7.1 which contains the fix https://github.com/mixpanel/mixpanel-swift/releases/tag/v2.7.1 Making archive a mono queue makes sense for reducing the cpu workload and memory footprint. But I didn't end up making archiveQueue async, mixpanel.archive is a public api that can be called independently by our users, sync is align with the behavior and the expectation from the system archive api. Internally, archive is always being called inside an async closure, so it shouldn't be a problem.

I'm closing this one now, as always, please feel free to post any issues and questions any time.