Skip to content
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

WIP: asset/album sync within an isolate #10668

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matejkramny
Copy link
Contributor

@matejkramny matejkramny commented Jun 27, 2024

Hi,

I've had a problem with my phone locking up, and suspected it's the number of assets being synced - #10030.
Looking at the code, it seems to be done on the UI thread. Isar has a recipe for multi isolate usage

I played around a bit with isolates (first time using it) and uploaded a profile build to my phone.

There's a lot less visual jank whilst the assets/albums are syncing. Importantly, the app doesn't permanently freeze anymore.

Have to say it's not without problems, the search page does some kind of syncing and it doesn't display properly at the moment, but i'm sure that's fixable.

Working in isolates is a bit challenging as the app has shared states. To that extent, I wasn't sure if the hashingService does something sequentially, which might pose a roadblock to this PR.

The demo server doesn't reproduce this problem, otherwise I would have made a demo video comparing before/after. I wonder if you could run the code and use the dart tools to measure FPS on a larger(than the demo) server so that you can see the impact.

It's currently a bit all over the place, just to test/validate the problem...

@matejkramny matejkramny marked this pull request as draft June 27, 2024 22:27
@bo0tzz bo0tzz requested review from fyfrey and martyfuhry June 28, 2024 08:27
@alextran1502
Copy link
Contributor

One nitpick about your PRs is that I see you often use abbreviations for variable names. Can you please write it out entirely instead? For example, receiver instead of rcv?

@fyfrey
Copy link
Contributor

fyfrey commented Jun 28, 2024

Thank you for this PR! I think this is a valuable short-term approach that should also make it easier to run these tasks in the background engine as well (mid-long term). I wanted to look into this already for some time :-)

Some important information on engines/isolates: We run two flutter engines (with currently 1 isolate per engine, but possible multiple). One engine is the foreground visible app, the other engine is invoked by the system to perform background backup (we plan to also put the syncing there).

We need to test the following:

  • The app (both background backup and foreground sync) still works correctly with this additional foreground isolate.
  • The main isolate is notified and re-renders if DB state changes in the DB isolate (e.g. syncing new photos causes the UI to rebuild and show them)

When moving all those DB operations to a separate isolate, we can also change any async DB ops to be synchronous (faster according to Isar docs and simplifies code). We need to be sure that any synchronous DB ops are only called from a non UI isolate, though.

The SyncService uses a locking mechanism to make sure operations are not running interleaved; with this PR we need to be sure that the SyncService methods are only ever called from the DB isolate.

Immediate feedback to your PR:
Please move all DB isolate related code to a new service (instead of using the asset service).
Please cleanup/remove unrelated changes to allow an unaltered comparison to check the functionality and review the code easier. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants