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

Switch to SQLite implementation of EntitiesRepository #6290

Merged
merged 44 commits into from
Aug 8, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Jul 25, 2024

Work towards #6012
Blocked by #6276

The major changes here:

  • Entities can now be reset from the project management settings
  • Local entities are now enabled by default
  • Local entities created before an update/follow-up form is downloaded will be available in forms downloaded later
  • Collect should support 100K item entity lists on most devices. For example with my Nexus 5X (which can barely handle the Play Store at this point):
    • Downloading form: ~2 mins (crashes after 30s when downloading manually however)
    • Loading form (with cached def): ~40s
    • Filtering entities by properties: ~2s

Why is this the best possible solution? Were any other approaches considered?

I think there are still a bunch of things we could do to get even better performance (both speed and memory wise) and to even make 1M item entity lists more credible, but this felt like a big enough milestone to merge before going any further.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The big risk here is to filling and downloading entity list forms as it's mostly changes to how local entities are stored.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@seadowg seadowg force-pushed the sqlite-entities branch 7 times, most recently from e878244 to c0bd11d Compare July 31, 2024 15:07
@seadowg
Copy link
Member Author

seadowg commented Jul 31, 2024

@getodk/testers this switches the implementation of entities storage over to a database. The goal here is to enable fast filtering for name= and <property>= filters on nodesets (like in a select) while also keeping memory use low (to avoid OOM errors). I think it'd be good to do an initial pass of the changes here with the performance testing project @lognaturel put together and get some first impressions of how we're doing. Here are some notes on the current performance expectations I'm seeing (so you can see if you get similar results):

  • Downloading an entity update or follow-up form will be slightly (a few seconds) slower than downloading the same form in an older (pre-entity) version of Collect and memory use will spike during (so there could be a risk of OOMs here). Downloading one of these forms for a subsequent time will actually be slightly slower again, but not a lot.
  • Loading entity update and follow-up forms will also be slightly slower than loading the same form in an older (pre-entity) version of Collect, but memory use will be much lower and will reduce after loading.
  • Querying for entity properties/name on an entity should be about as fast as doing so with the same forms in an older (pre-entity) version of Collect and memory use shouldn't increase by much at all when moving between these screens.

I got to this kind of performance testing against the "100k Entities Filter" form with an emulator (running on a MacBook) so it could be that the results you see on a device are quite different. I'll have a play with an older phone as well, but thought it'd be good to get us all looking at it! What's for certain is I'm definitely seeing better overall performance (faster and lower memory use) compared than using these forms with what's currently on master.

EDIT: You'll also be able to clear entities from the standard project reset settings now.

@dbemke
Copy link

dbemke commented Aug 2, 2024

We tried 100k entities on Android 10 (Redmi devices) and it seems to be working better. On 2 devices with Android 10 no probelms on downloading the form, refreshing the list of blank forms or filling the form. On Android 14 it works well, too.

@seadowg
Copy link
Member Author

seadowg commented Aug 5, 2024

We tried 100k entities on Android 10 (Redmi devices) and it seems to be working better. On 2 devices with Android 10 no probelms on downloading the form, refreshing the list of blank forms or filling the form. On Android 14 it works well, too.

Great thanks! I think that's probably good enough for us to go ahead with reviewing/merging this, and then we can start more in depth performance testing/improvements after.

@seadowg seadowg requested a review from grzesiek2010 August 7, 2024 13:44
@seadowg seadowg requested a review from grzesiek2010 August 8, 2024 09:38
entities/build.gradle.kts Outdated Show resolved Hide resolved
@seadowg seadowg requested a review from grzesiek2010 August 8, 2024 14:01
@grzesiek2010 grzesiek2010 merged commit c02841e into getodk:master Aug 8, 2024
6 checks passed
@seadowg seadowg deleted the sqlite-entities branch August 8, 2024 14:43
@WKobus
Copy link

WKobus commented Aug 13, 2024

Tested with Success

Verified on device with Android 14

Verified cases:

  • Resetting entities in project management
  • Performance on 100k entities form
  • Downloading update form after creating local entity
  • Regression check on entities

@dbemke
Copy link

dbemke commented Aug 13, 2024

Tested with Success

Verified on a device with Android 10 and Android 8.1

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.

4 participants