Repository navigation
perf: load and save drawings off the UI thread - #3
Merged
Merged
Conversation
getStrokes serialized every point to JSON on the UI thread on both platforms, and setStrokes built a stroke per point there too. Both now run on a worker thread and answer back on the UI thread, the way the image export already did. The Android export executor is shared, so the document commands queue behind it instead of building a second thread. The iOS brush map grew for the whole session and was never pruned, because clear and undo leave entries that undo itself still needs. Keeping the most recent 1000 bounds the memory while leaving undo intact.
setStrokes now builds its strokes on a worker thread, so a clear, undo, redo, getStrokes or exportImage sent right after it could run before the load landed. Commands now go through a main-thread queue: they still run at once, and only wait while a setStrokes is loading. Exports go back to their own thread, because they can wait there for the background image and document commands should not wait behind that. Answers post through the main looper, since View.post holds them until a detached view is attached again. Co-authored-by: Aziz Becha <aziz07becha@gmail.com>
setStrokes decoded on a concurrent global queue, so two loads could land out of order, and a clear, undo, redo, getStrokes or exportImage sent after a load could run before it. Document work now runs on a private serial queue, and commands go through a main-thread queue that only waits while a load is pending. An export that waits for the background image keeps the drawing it was asked for. The brush map drops its 1000-entry cap. An entry is about 116 bytes while undo history holds about 4.8 KB per 150-point stroke, and the cap gave evicted strokes the current brush on save. Co-authored-by: Aziz Becha <aziz07becha@gmail.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
This takes #1 by @azizbecha onto current main, with the fixes from its review.
getStrokesandsetStrokesnow run off the UI thread on both platforms. On a 300-stroke drawing with 45,000 points, they blocked the main thread for 65 ms and 89 ms on the iOS simulator. On the Android emulator it was 160 ms and 434 ms.Commands still apply in the order JS sends them. A
clear,undo,redo,getStrokesortoImagesent after asetStrokeswaits for the load to finish, sosetStrokes(doc); clear()clears the loaded drawing, and two loads apply in the order JS sent them. When nothing is loading, commands run right away as before. Each platform has a small main-thread queue for this,CommandQueueon Android andINKCommandQueueon iOS. Sending every command through the worker thread would have made them wait behind exports, and on Android an export can wait there for the background image.Android answers now post through the main looper.
View.postholds a runnable until the view attaches to a window again, so a call made while the canvas was in an inactive tab would time out.On iOS, an export that waits for the background image keeps the drawing from the moment of the call, so
toImage(); clear()no longer exports the cleared canvas.The iOS brush map has no cap. An entry costs about 116 bytes, while undo history already holds about 4.8 KB for each 150-point stroke. A capped map also gave the strokes it evicted the current brush on save.
CommandQueueTestcovers the Android queue on the JVM.InkpadViewCommandsTestis an instrumented test on a realInkpadView: clear, undo andgetStrokesbehind a pending load, two loads, a failed load, and a detached view.iosCommandQueue.test.tsbuilds the iOS queue with clang and runs it. The e2e suite in CI covers the iOS view itself.Supersedes #1.