-
Notifications
You must be signed in to change notification settings - Fork 20
Fix high memory usage on apple devices #305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
rkistner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some general comments:
- Since most of the code is a fork from the ktor-client-darwin library, we need to include a copy of their license.
- There is quite a lot of code here from the fork. Is there an easy way to view the difference? And how would we keep it up to date with the upstream version in the future? (Can we get the fix merged upstream?)
- The bounded channel has a capacity of "64". What does this mean in practice? Does this translate to a practical limit in MB?
simolus3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest question is whether this actually works. I've explored a different approach that calls suspend() on the NSURLSession download task when the channel is full, and the framework implementation just completely ignored that. So it's not obvious to me whether blocking the receive thread is enough to implement backpressure (since most of the OS APIs are asynchronous).
Steven has created an aggressive repro for this, can you check whether the client fork fixes that? You can start the server, then run the iOS app in a simulator. At least in my attempts, memory usage was growing unbounded for about 40% of all app starts (with my approach, it looks like there may be a race condition in the underlying HTTP client implementation from the OS).
| 1. **NSURLSession accumulates all response chunks** into a single `NSData` object | ||
| 2. **Ktor converts the entire NSData to ByteArray** - allocating another full copy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't what actually happens - if it were the case, streaming sync would be completely broken. We still see individual lines being emitted by ktor, it's just that backpressure doesn't reach the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, ignore this file. It was a hold over from my initial investigation into how sessions were handled - very wrong indeed.
...nal/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinTaskHandler.kt
Outdated
Show resolved
Hide resolved
internal/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/DarwinClientEngine.kt
Outdated
Show resolved
Hide resolved
...nal/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinTaskHandler.kt
Outdated
Show resolved
Hide resolved
| * Copyright 2014-2019 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license. | ||
| */ | ||
|
|
||
| package io.ktor.client.engine.darwin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid linker issues if someone also depends on the upstream package in their app, we should give all of these files a different package name (e.g. com.powersync.internal.client_fork.darwin)
I did keep all license headers in the files, but you are right I included it as a standalone.
A fix upstream would of been ideal. This is being tracked in this issue, but I think the idea was to have this as a temporary fix, until the fix was merged upstream.
I tested channel capacities of 8, 64, and 256. The idea being to initially using The single chunk capacity had the lowest memory usage ~600 MB and showed similar performance (operations/s) in my hacky benchmarks, even for short bursts. So not sure if my theory or testing is flawed. But if the goal is to lower memory usage, single chunk makes sense. |
Using a channel capacity of 1:
|
@joshuabrink Do I understand correctly that this means 700-900MB vs. multiple GB in reproduction of the original reported issue? |
|
@cahofmeyr Yes. The only caveat is performance. I've done a couple comparisons of the operation/s of this PR v.s. the original implementation v.s. the csqlite implementation. Taking into account that my benchmarks are not 100% accurate - this runBlocking PR is still orders of magnitude slower than both. I do have these benchmarks available if anyone wants to take a look. |
Add a custom Ktor Darwin HTTP engine to the repository with bounded channel backpressure to prevent out-of-memory crashes when processing large sync payloads on iOS/macOS.
Problem
NSURLSession delivers data faster than it can be processed, causing unbounded buffering in the upstream Ktor Darwin engine's channel. This results in multi-GB memory spikes and OOM crashes on large sync operations.
Solution
Implemented a custom Darwin client fork with a bounded channel (capacity: 64) and backpressure handling via
runBlocking. This naturally throttles NSURLSession's data delivery rate to match processing speed, eliminating memory spikes.Changes
internal/ktor-client-darwin/with modifiedDarwinTaskHandler.kt