Make DataCache Sendable and drain it with a single writer task - #925
Draft
kean wants to merge 9 commits into
Draft
Make DataCache Sendable and drain it with a single writer task#925kean wants to merge 9 commits into
kean wants to merge 9 commits into
Conversation
The staged changes are now flushed by a `Task` instead of `DispatchQueue.asyncAfter`, and all of the mutable state moved behind a single `OSAllocatedUnfairLock`, which makes the class `Sendable` rather than `@unchecked Sendable`.
… task DataCaching.cachedData(for:) and containsData(for:) are now async, and the pipeline reads the disk cache off the ImagePipelineActor. DataCache replaces the scheduled flush with a single self-terminating writer task that drains the staging area in a loop: writes batch up automatically under load, data reaches the disk in milliseconds instead of after a fixed 1-second delay, and there is no timer state to race on. flush() and sweep() are now async and wait by awaiting the writer task's value, which escalates the utility-QoS writer to the caller's priority, so an explicit flush performs the same as the old queue.sync. flush(for:) is removed.
The file operations are synchronous and can block for a long time, so running them in a Task occupied a cooperative pool thread for the entire duration of a write. The writer task now hops into a serial dispatch queue and drains the pending work there, which is what GCD threads are for. The queue also guarantees that the disk operations never overlap. Replace it with an actor using DispatchSerialQueue as its SerialExecutor once the deployment target reaches iOS 17. The queue is .default, not .utility: a block submitted with async never gets its priority escalated, unlike the queue.sync the cache used before, so at .utility the caller of flush() waits on throttled I/O. At .utility the write benchmarks regressed by 40-70%; at .default they match.
The writer started on the very first staged change, so the batching depended on how slow the disk happened to be: a trickle of writes spaced further apart than a single disk operation got a task hop, a staging snapshot, and a separate pass over the disk each, and the repeated writes to the same key were no longer collapsed into one. Restore the 1 second window the cache had before the rewrite. The writer waits through it before draining, so the changes made within it are written in one pass. Only the automatic drain is throttled – flush() and sweep() perform the work themselves and never wait on the window.
flushWaitsForPendingWrites and concurrentFlushesAllReturn suspended the I/O and resumed it while a flush was pending, but suspendIO() only gates the writer – flush() submits to the I/O queue itself, so both tests passed without ever reaching the scenario in their comments and would still pass with resumeIO() deleted. Stage the changes behind a long flush interval instead, which is a state the cache actually gets into. The suite also never cleaned up after itself: every run left ~21 cache directories in ~/Library/Caches for good, and the sustained traffic tests wrote an unbounded number of files into them. Recycle the traffic keys, remove the directories when the tests are done with them, and stop the writer first so that it can't re-create one after the fact. Bound the number of DataCache operations the thread safety test runs at a time the way the OperationQueue it replaced did, add the missing coverage for the writer draining on its own, and rename writeWithFlushIndividual, which stopped measuring a per-key flush when flush(for:) was removed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DataCachewas the last type in the library still coordinating its work with GCD: a serialDispatchQueue, anNSLockfor the staging area,asyncAfterfor the flush and sweep timers, and@unchecked Sendableto paper over the rest.It now keeps all of its mutable state – the staging area and the configuration – behind a single
OSAllocatedUnfairLock, which makes itSendable. The 1-second flush timer is replaced by a single self-terminating writerTaskthat drains the staging area in a loop, so the writes still batch up under load but reach the disk in milliseconds, and there is no timer state to race on.The blocking file operations run on a serial dispatch queue. Doing them in a task would occupy a cooperative pool thread for the whole duration of a write, so the writer hops into the queue once and drains everything there – blocking work is what GCD threads are for. The queue is
.default, not.utility: a block submitted withasyncnever gets its priority escalated, unlike thequeue.syncthe cache used before, so at.utilitythe caller offlush()waits on throttled I/O.Two bugs fixed along the way:
sizeLimit,sweepInterval,isSweepEnabled, andtrimRatiowere plainvars written by the client and read from the I/O queue. They now live in the lockedState;sweepDelayandonSweepCompletedbecamelet, since only the internal test initializer ever set them.initperformed disk I/O on the calling thread.scheduleSweep()read and JSON-decoded.data-cache-infobefore returning, i.e. on the main thread forImagePipeline.Configuration.withDataCache. The check moved into the writer, soonSweepCompletednow also fires only when a sweep actually runs.One more trap worth recording:
Task.sleepat.backgroundpriority is subject to timer coalescing, andscheduledSweepUpdatesMetadatawent from 0.034s to 11.7s. The sweep task runs at.utility, with a comment explaining why.Breaking:
DataCache.flush()andDataCache.sweep()are nowasync.DataCache.flush(for:)andDataCache.queueare removed – the queue is an implementation detail now.DataCacheTestsusedqueueto suspend the I/O, sowithSuspendedIOmoved into the source as an internal test hook.The
DataCachingprotocol and the staging area are unchanged. The per-change IDs in the staging area look like ceremony, but they are what lets the writer drop the state lock while writing and then remove only the entries that weren't replaced meanwhile — that's the property that keeps reads parallel to writes.To test
NukeTestsscheme – 991 tests pass, including all 47DataCacheTests.NukeThreadSafetyTestsscheme – 7 tests pass.NukePerformanceTests/DataCachePeformanceTests– the write benchmarks are unchanged.