Include DynamicPreloadExtra in formDef only if a form contains search or pulldata

grzesiek2010 commented 1 month ago

Closes #6447

In issue #6446, I mentioned that the problem with initializing DynamicPreloadExtra is likely not the main cause of the memory issues. However, when this exception is thrown, it indicates that we cannot read the form from cache and must load it from the file instead. This process consumes more memory and could be one of the factors contributing to the numerous out-of-memory issues we are experiencing.

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

As I mentioned in the issue, I cannot reproduce it and don’t fully understand why DynamicPreloadExtra sometimes cannot be instantiated, but I have some suspicions. Initially, I thought the issue might be related to ProGuard and the app shrinking process. We need an empty constructor to instantiate DynamicPreloadExtra using the newInstance() method in javarosa, and I wondered if that constructor had been removed. However, after installing the app version from the store, I didn’t encounter that exception, so that can’t be the cause.

Since this issue occurs for only some users, I suspect that newInstance() may have trouble creating an object from a Kotlin class. I know this sounds unusual, but after working on issue #6364, I have become more open to the idea that such unexpected issues can arise.

Although I can’t be certain about the root cause of the issue, I have a plan to at least mitigate it. For now, we add DynamicPreloadExtra to formDef regardless of whether a form uses pulldata or search. There is a true/false value that determines this, but I believe it is unnecessary. For instance, EntityFormExtra is only added if the form actually contains entities (see: EntityFormParseProcessor.java#L63). I think DynamicPreloadExtra should operate in the same way and thanks to that it should not need to store any value. If it is present, it indicates that the form contains search or pulldata, if it is absent, then it does not.

If I’m correct that newInstance() may have difficulties creating objects from Kotlin classes, these changes might help, as the empty constructor is now the primary one. Could this have been the issue?

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?

This requires testing forms with search/pulldata.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

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

WKobus commented 1 month ago

Tested with Success

Verified on device with Android 14

Verified cases:

Forms with search and pulldata
Pulldata in entity forms

dbemke commented 1 month ago

Tested with Success

Verified on device with Android 10

getodk / collect