Optimize redownloading entity forms #6376

seadowg · 2024-08-26T19:10:38Z

Closes #6344
Closes #6389
~~Blocked by #6372~~

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

There are two keys changes here that lower subsequent download times (with or without changes):

Entities that haven't changed aren't saved to the database as an update any longer
Entity list CSVs that exactly match the previously processed list are not reprocessed

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 main changes and risks are again all around form update for entity follow-up and update forms.

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 · 2024-08-30T09:32:13Z

@lognaturel I've got a few more changes to make here (the current optimization around skipping extra saves is probably buggy), but it'd be interesting to get your Pixel 3a numbers here. I'd like to update the benchmark targets so that it passes on both the FP3 and that device.

seadowg · 2024-09-02T11:05:00Z

entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt

@@ -184,31 +184,6 @@ class LocalEntityUseCasesTest {
        assertThat(songs[0].branchId, equalTo(offline.branchId))
    }

-    @Test
-    fun `updateLocalEntitiesFromServer overrides offline version if the online version is the same`() {


@lognaturel I'm not sure if this is behaviour we want or not so I wanted to call it out. It seems much simpler to me for the client to assume that if its local version and the server version of an entity are the same that the data (label and properties) will also be the same. I think this is the case when using Central as edits/fixing conflicts would always increment the version? If that's not the case and Central can return a matching version number with different data, then I can call this out with a more explicit test.

@lognaturel and I chatted about this, and we actually want to do the opposite so that the entity "metadata" and data are always in sync. We also want to make sure that trunkVersion and branchId make sense in cases where the server has different data, but the same version. I'll make changes to this.

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt

...ct_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt

entities/src/test/java/org/odk/collect/entities/LocalEntityUseCasesTest.kt

entities/src/main/java/org/odk/collect/entities/storage/Entity.kt

collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt

entities/src/main/java/org/odk/collect/entities/storage/Entity.kt

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt

...ct_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt

lognaturel · 2024-09-04T21:31:41Z

Pixel 3A:

Downloading form with http cache: 37s
Downloading form second time with http cache: 2s
Loading form first time: 1s
Loading form second time: 1s
Filtering select: 1s

When manually opening the form after, I got the following exception

java.lang.RuntimeException: StrictMode ThreadPolicy violation
     	at android.os.StrictMode$AndroidBlockGuardPolicy.onThreadPolicyViolation(StrictMode.java:1876)
     	at android.os.StrictMode$AndroidBlockGuardPolicy.handleViolationWithTimingAttempt(StrictMode.java:1733)
     	at android.os.StrictMode$AndroidBlockGuardPolicy.startHandlingViolationException(StrictMode.java:1704)
     	at android.os.StrictMode$AndroidBlockGuardPolicy.onCustomSlowCall(StrictMode.java:1626)
     	at android.os.StrictMode.noteSlowCall(StrictMode.java:2694)
     	at org.odk.collect.db.sqlite.DatabaseConnection.getReadableDatabase(DatabaseConnection.kt:36)
     	at org.odk.collect.android.database.entities.DatabaseEntitiesRepository.listExists(DatabaseEntitiesRepository.kt:317)
     	at org.odk.collect.android.database.entities.DatabaseEntitiesRepository.getAllByProperty(DatabaseEntitiesRepository.kt:224)
     	at org.odk.collect.entities.javarosa.intance.LocalEntitiesInstanceAdapter.queryEq(LocalEntitiesInstanceAdapter.kt:57)
     	at org.odk.collect.entities.javarosa.filter.LocalEntitiesFilterStrategy.filter(LocalEntitiesFilterStrategy.kt:46)
     	at org.javarosa.core.model.condition.EvaluationContext.filterWithPredicate(EvaluationContext.java:382)
     	at org.javarosa.core.model.condition.EvaluationContext.filterWithPredicate(EvaluationContext.java:377)
     	at org.javarosa.core.model.condition.EvaluationContext.expandReferenceAccumulator(EvaluationContext.java:343)
     	at org.javarosa.core.model.condition.EvaluationContext.expandReference(EvaluationContext.java:250)
     	at org.javarosa.core.model.condition.EvaluationContext.expandReference(EvaluationContext.java:219)
     	at org.javarosa.xpath.expr.XPathPathExprEval.eval(XPathPathExprEval.java:34)
     	at org.javarosa.xpath.expr.XPathPathExpr.eval(XPathPathExpr.java:213)
     	at org.javarosa.xpath.XPathConditional.evalNodeset(XPathConditional.java:94)
     	at org.javarosa.core.model.ItemsetBinding.getChoices(ItemsetBinding.java:125)
     	at org.javarosa.form.api.FormEntryPrompt.getSelectChoices(FormEntryPrompt.java:222)
     	at org.javarosa.form.api.FormEntryPrompt.getAnswerValue(FormEntryPrompt.java:98)
     	at org.javarosa.form.api.FormEntryPrompt.getAnswerText(FormEntryPrompt.java:154)
     	at org.odk.collect.android.formhierarchy.QuestionAnswerProcessor.getQuestionAnswer(QuestionAnswerProcessor.kt:28)
     	at org.odk.collect.android.formhierarchy.FormHierarchyActivity.refreshView(FormHierarchyActivity.java:672)
     	at org.odk.collect.android.formhierarchy.FormHierarchyActivity.refreshView(FormHierarchyActivity.java:591)
     	at org.odk.collect.android.formhierarchy.FormHierarchyActivity.onCreate(FormHierarchyActivity.java:278)
     	at android.app.Activity.performCreate(Activity.java:8054)
     	at android.app.Activity.performCreate(Activity.java:8034)
     	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1341)
     	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3688)
     	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3864)
     	at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:103)
     	at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135)
     	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95)
     	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2253)
     	at android.os.Handler.dispatchMessage(Handler.java:106)
     	at android.os.Looper.loopOnce(Looper.java:201)
     	at android.os.Looper.loop(Looper.java:288)
     	at android.app.ActivityThread.main(ActivityThread.java:7870)
     	at java.lang.reflect.Method.invoke(Native Method)
     	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
     	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
     Caused by: android.os.strictmode.CustomViolation: Accessing readable DB
     	at android.os.StrictMode$AndroidBlockGuardPolicy.onCustomSlowCall(StrictMode.java:1626) 
     	at android.os.StrictMode.noteSlowCall(StrictMode.java:2694) 
     	at org.odk.collect.db.sqlite.DatabaseConnection.getReadableDatabase(DatabaseConnection.kt:36) 
     	at org.odk.collect.android.database.entities.DatabaseEntitiesRepository.listExists(DatabaseEntitiesRepository.kt:317) 
     	at org.odk.collect.android.database.entities.DatabaseEntitiesRepository.getAllByProperty(DatabaseEntitiesRepository.kt:224) 
     	at org.odk.collect.entities.javarosa.intance.LocalEntitiesInstanceAdapter.queryEq(LocalEntitiesInstanceAdapter.kt:57) 
     	at org.odk.collect.entities.javarosa.filter.LocalEntitiesFilterStrategy.filter(LocalEntitiesFilterStrategy.kt:46) 
     	at org.javarosa.core.model.condition.EvaluationContext.filterWithPredicate(EvaluationContext.java:382) 
     	at org.javarosa.core.model.condition.EvaluationContext.filterWithPredicate(EvaluationContext.java:377) 
     	at org.javarosa.core.model.condition.EvaluationContext.expandReferenceAccumulator(EvaluationContext.java:343) 
     	at org.javarosa.core.model.condition.EvaluationContext.expandReference(EvaluationContext.java:250) 
     	at org.javarosa.core.model.condition.EvaluationContext.expandReference(EvaluationContext.java:219) 
     	at org.javarosa.xpath.expr.XPathPathExprEval.eval(XPathPathExprEval.java:34) 
     	at org.javarosa.xpath.expr.XPathPathExpr.eval(XPathPathExpr.java:213) 
     	at org.javarosa.xpath.XPathConditional.evalNodeset(XPathConditional.java:94) 
     	at org.javarosa.core.model.ItemsetBinding.getChoices(ItemsetBinding.java:125) 
     	at org.javarosa.form.api.FormEntryPrompt.getSelectChoices(FormEntryPrompt.java:222) 
     	at org.javarosa.form.api.FormEntryPrompt.getAnswerValue(FormEntryPrompt.java:98) 
     	at org.javarosa.form.api.FormEntryPrompt.getAnswerText(FormEntryPrompt.java:154) 
     	at org.odk.collect.android.formhierarchy.QuestionAnswerProcessor.getQuestionAnswer(QuestionAnswerProcessor.kt:28) 
     	at org.odk.collect.android.formhierarchy.FormHierarchyActivity.refreshView(FormHierarchyActivity.java:672) 
     	at org.odk.collect.android.formhierarchy.FormHierarchyActivity.refreshView(FormHierarchyActivity.java:591) 
     	at org.odk.collect.android.formhierarchy.FormHierarchyActivity.onCreate(FormHierarchyActivity.java:278) 
     	at android.app.Activity.performCreate(Activity.java:8054) 
     	at android.app.Activity.performCreate(Activity.java:8034) 
     	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1341) 
     	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3688) 
     	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:3864) 
     	at android.app.servertransaction.LaunchActivityItem.execute(LaunchActivityItem.java:103) 
     	at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135) 
     	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95) 
     	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2253) 
     	at android.os.Handler.dispatchMessage(Handler.java:106) 
     	at android.os.Looper.loopOnce(Looper.java:201) 
     	at android.os.Looper.loop(Looper.java:288) 
     	at android.app.ActivityThread.main(ActivityThread.java:7870) 
     	at java.lang.reflect.Method.invoke(Native Method) 
     	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548) 
     	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)

I believe that was because an update was happening in the background as I tried to open, does that sound right (so related to #6232)?

seadowg · 2024-09-05T07:21:53Z

When manually opening the form after, I got the following exception

Interesting! It looks to me like we'll get that for any filtered entity list when opening the view hierarchy (in debug builds) because select choices are loaded on the UI thread there. I'm guessing fixing this is going to be a lot of work, so I might have to look at disabling the strict mode check there for now.

seadowg · 2024-09-05T08:27:19Z

I'm guessing fixing this is going to be a lot of work, so I might have to look at disabling the strict mode check there for now.

Digging into it, it doesn't feel like the best use of my time to work around the broken form hierarchy at the moment. I'm going to pull this out as an issue as we can always just disable StrictMode for a bit if we need to.

grzesiek2010

There is still one question unanswered #6376 (comment) plus this test name that should be improved.

collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt

dbemke · 2024-09-10T06:58:58Z

After changes in the PR the update and follow-up forms don't get updated unless there's a change in entities CSV. When I go to "Download form" I see that the subtext "This is an update to a form you have" even in a case when these forms won't get updated cause there weren't any changes. Should this subtext appear when there aren't any changes in the CSV?

seadowg · 2024-09-10T07:06:39Z

Should this subtext appear when there aren't any changes in the CSV?

Good question! That was the case before the PR right? There was no visible update to the forms if the CSV wasn't updated (we just don't do any extra work we don't have to now). I think that's expected right now. As far as I'm concerned, we're not really optimizing the entities experience for manual downloads (although they are very useful for testing different scenarios), but it might be something we want to tweak if we do see a lot of entities use with them.

dbemke · 2024-09-10T07:14:21Z

That was the case before the PR right?

Yes

dbemke · 2024-09-10T12:51:30Z

Tested with Success!

Verified on a device with Android 10

Verified Cases:

issues SQlite syntax error- crash after refreshing list of blank forms containg entity form with property with "." #6344 SQlite error- crash after refreshing the list of blank forms #6389 don't occur
adding properties with symbols in Central and refreshing the list of blank form
adding period and hyphen in properties in form with the new entity spec
downloading updates to entity form with/without changes in dataset CSV
downloading new versions of entities forms

WKobus · 2024-09-10T12:52:21Z

Tested with Success

Verified on device with Android 14

seadowg added the blocked label Aug 26, 2024

seadowg force-pushed the optimize-upload branch from c37dc4c to eb4bb68 Compare August 28, 2024 13:00

seadowg mentioned this pull request Aug 29, 2024

Optimize form download with entities #6372

Merged

6 tasks

seadowg added 13 commits August 30, 2024 09:56

Don't write to DB when entities are the same

df241ab

Lower benchmark target to lowest passing value

37b2de5

Make test stricter

8a1996a

Spike out skipping server entity update for same file

097d20c

Maintain first download time

e0bdfb3

Change method name

d1056cc

Use md5 prefix to allow for other version formats

a07885c

Update testing and remove unused code

02a84c4

Simplify #save by only allowing one list

fd37e32

Move list out of Entity

351113e

Update column name

b39579b

Update tests for new #save signature

0798d55

Lower benchmarks to lower passing values for form loading and querying

a3a102b

seadowg force-pushed the optimize-upload branch from a6b69a3 to a3a102b Compare August 30, 2024 08:56

seadowg removed the blocked label Aug 30, 2024

seadowg added 4 commits September 2, 2024 10:23

Fix updates with special characters in property names

3801322

Don't repurpose entity data object for parsing CSV

b5f5c0e

Reimplement not saving existing versions optimization

eac1d30

Optimize imports

67576c0

seadowg commented Sep 2, 2024

View reviewed changes

seadowg marked this pull request as ready for review September 2, 2024 11:05

seadowg requested a review from grzesiek2010 September 2, 2024 11:05

Don't override entity data in equal version case

4667073

seadowg force-pushed the optimize-upload branch from a8af2c0 to 4667073 Compare September 2, 2024 12:25

seadowg mentioned this pull request Sep 3, 2024

Allow null labels in follow up forms #6393

Merged

6 tasks

grzesiek2010 requested changes Sep 3, 2024

View reviewed changes

seadowg requested a review from grzesiek2010 September 4, 2024 11:45

grzesiek2010 reviewed Sep 4, 2024

View reviewed changes

entities/src/main/java/org/odk/collect/entities/storage/Entity.kt Outdated Show resolved Hide resolved

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt Show resolved Hide resolved

grzesiek2010 requested changes Sep 4, 2024

View reviewed changes

...ct_app/src/main/java/org/odk/collect/android/database/entities/DatabaseEntitiesRepository.kt Show resolved Hide resolved

Add more detail to trunkVersion comment

3f4b1cc

seadowg requested a review from grzesiek2010 September 4, 2024 13:26

Fix zero entities #save case

77467a5

seadowg mentioned this pull request Sep 4, 2024

Mismatch error after finalizing forms, adding property in Central and refreshing list of blank forms #6396

Closed

Add Pixel 3 numbers to benchmark

f38ec1e

grzesiek2010 requested changes Sep 5, 2024

View reviewed changes

collect_app/src/test/java/org/odk/collect/android/entities/EntitiesRepositoryTest.kt Outdated Show resolved Hide resolved

seadowg added 2 commits September 5, 2024 11:59

Fixed overriding local version when it's dirty

449ecc6

Update test name

13c80b0

seadowg requested a review from grzesiek2010 September 5, 2024 11:22

grzesiek2010 requested changes Sep 5, 2024

View reviewed changes

entities/src/main/java/org/odk/collect/entities/LocalEntityUseCases.kt Outdated Show resolved Hide resolved

lognaturel added the high priority Should be looked at before other PRs/issues label Sep 5, 2024

Separate version greater than and equal case

f5c8141

seadowg requested a review from grzesiek2010 September 6, 2024 08:58

grzesiek2010 approved these changes Sep 6, 2024

View reviewed changes

grzesiek2010 merged commit a153d02 into getodk:master Sep 6, 2024
6 checks passed

grzesiek2010 added the needs testing label Sep 6, 2024

seadowg deleted the optimize-upload branch September 6, 2024 10:22

WKobus added behavior verified and removed needs testing labels Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize redownloading entity forms #6376

Optimize redownloading entity forms #6376

seadowg commented Aug 26, 2024 •

edited

Loading

seadowg commented Aug 30, 2024

seadowg Sep 2, 2024

seadowg Sep 3, 2024

lognaturel commented Sep 4, 2024 •

edited

Loading

seadowg commented Sep 5, 2024

seadowg commented Sep 5, 2024

grzesiek2010 left a comment

dbemke commented Sep 10, 2024

seadowg commented Sep 10, 2024 •

edited

Loading

dbemke commented Sep 10, 2024

dbemke commented Sep 10, 2024

WKobus commented Sep 10, 2024

Optimize redownloading entity forms #6376

Optimize redownloading entity forms #6376

Conversation

seadowg commented Aug 26, 2024 • edited Loading

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

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?

Before submitting this PR, please make sure you have:

seadowg commented Aug 30, 2024

seadowg Sep 2, 2024

Choose a reason for hiding this comment

seadowg Sep 3, 2024

Choose a reason for hiding this comment

lognaturel commented Sep 4, 2024 • edited Loading

seadowg commented Sep 5, 2024

seadowg commented Sep 5, 2024

grzesiek2010 left a comment

Choose a reason for hiding this comment

dbemke commented Sep 10, 2024

seadowg commented Sep 10, 2024 • edited Loading

dbemke commented Sep 10, 2024

dbemke commented Sep 10, 2024

WKobus commented Sep 10, 2024

seadowg commented Aug 26, 2024 •

edited

Loading

lognaturel commented Sep 4, 2024 •

edited

Loading

seadowg commented Sep 10, 2024 •

edited

Loading