fix: handle screenshot memory leak at low throttle intervals(<=1s)#532
Conversation
PostHog/PostHogQueue.swift
Outdated
| private var flushThreshold: Int { | ||
| endpoint == .snapshot ? 1 : config.flushAt | ||
| } | ||
| private var flushBatchSize: Int { | ||
| endpoint == .snapshot ? 1 : config.maxBatchSize | ||
| } | ||
| private var flushTimerInterval: TimeInterval { | ||
| endpoint == .snapshot ? 1 : config.flushIntervalSeconds | ||
| } |
There was a problem hiding this comment.
Hey @KristijanKocev thank you for this! Can we check if this is indeed needed? My hunch says the autoreleasepool changes should already be enough.
I’m a bit wary of aggressive disk flushing. Even if that path is gated, it still means more frequent network sends, which could keep the radio active more often and increase overhead. This may improve one symptom while quietly making battery or network behavior worse.
There was a problem hiding this comment.
You might be right, I will do some testing today and report back.
There was a problem hiding this comment.
Just removed the queue flushing and it didn't have much of an effect since the memory leak was still gone, so you were right.
| private func startSnapshotIfPossible() -> Bool { | ||
| snapshotStateLock.withLock { | ||
| if snapshotInFlight { | ||
| snapshotPending = true | ||
| return false | ||
| } | ||
|
|
||
| snapshotInFlight = true | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| private func finishSnapshot() { | ||
| let shouldScheduleNext = snapshotStateLock.withLock { | ||
| snapshotInFlight = false | ||
| let shouldScheduleNext = snapshotPending | ||
| snapshotPending = false | ||
| return shouldScheduleNext | ||
| } | ||
|
|
||
| if shouldScheduleNext { | ||
| DispatchQueue.main.async { [weak self] in | ||
| self?.snapshot() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func resetSnapshotState() { | ||
| snapshotStateLock.withLock { | ||
| snapshotInFlight = false | ||
| snapshotPending = false | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Apologies @KristijanKocev, can we try without these changes as well and see how the improvements of autoreleasepool perform alone?
There was a problem hiding this comment.
I should have scoped the pr a bit better, that's my bad.
I removed them and yea just the autoreleasepool takes care of the leak.
Btw i am not sure if it's a known issue but on older iphones like 13 and older, every time a screenshot gets taken there is a mini freeze which can be easily noticed if you're scrolling.
These were some of my attempts which i've been using in prod to some effect, together with this pr #536
I'm not yet fully satisfied however which is why I will keep the changes to myself until am I sure.
There was a problem hiding this comment.
Yeah, it's been reported again and the PR you linked attempts to improve on that. If there are any ideas from what you've tried so far please do share. You can email me directly at yiannis at posthog dot com if you'd prefer.
Moving the whole screenshot capture to a bg thread helps but I'd like to avoid that for as long as possible cause it can have unpredictable side effects.
There was a problem hiding this comment.
I will keep toying with it, nothing so far has made much of a difference.
I tried switching to wireframe mode but that also requires a bit of work so that it can function for react native apps.
It might also have something to do with me still being on the old architecture but I haven't yet migrated to test that.
ioannisj
left a comment
ioannisj
left a comment
There was a problem hiding this comment.
lg, just pushed a minor fix
2 participants
💡 Motivation and Context
We started using posthog-react-native-session-replay since its release and it worked well until after a react-native version upgrade in october last year rendered the session replay unusable for us and I had to disable it.
Today I wanted to see if it has been fixed and the same issue appeared.
What would happen is that with a throttle delay of 1000 ms or less, the screenshot based session replay caused the memory usage to keep climbing, without ever releasing memory. Eventually this causes a crash as iOS kills the app for using too much memory.
This was observed on an iphone 13, 16 and 17 pro.
With a higher interval such as 5 seconds, the memory usage increased by ~25mb every 5 seconds and dropped shortly after, which is the expected behaviour.
This told me that the screenshot pipeline was not able to complete a cycle and release memory before it had to start all over again.
This change reduces that buildup in three places:
The result was that I could set the throttle delay to 1 second again for good quality recordings while memory usage remained stable.
💚 How did you test it?
📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset filereleaselabel to the PR