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

Optimize entity form load times #6318

Merged
merged 14 commits into from
Aug 20, 2024
Merged

Optimize entity form load times #6318

merged 14 commits into from
Aug 20, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Aug 8, 2024

Blocked by #6290

Load times should now be under 5s on most devices.

I've also added a new EntitiesBenchmarkTest that can be run manually against different real devices. This will let us check that we don't regress performance around entity forms.

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

I did some profiling on bottlenecks for form load time, and it appeared the most significant was the cost involved in performing DatabaseEntitiesRepository#getEntities. Digging in further, it appears this was due to the time it takes to construct a List containing all the entities (in a list) from the Cursor query as it has to move through many items and for large lists this results in filling the cursor window multiple times.

To alleviate that bottleneck, I've introduced getCount and getByIndex methods that can be used when loading a form (where we just do a "partial" parse) instead of needing to load all the entities.

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 should just affect entity forms. Entity forms that have a select containing every entity on the first screen will not be any faster as this still requires the app to load all entities for a list.

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 added the blocked label Aug 8, 2024
@seadowg seadowg removed the blocked label Aug 8, 2024
@seadowg seadowg changed the title Add benchmark for entities Optimize entity form load times Aug 14, 2024
README.md Show resolved Hide resolved
@lognaturel
Copy link
Member

I'm able to run this on my Galaxy A13 by rebasing on #6353 and making the following change to BeforeProjectsInstallDetector:

        // A bg_startup_tracing key sometimes appears for some devices, maybe related to WebViews
        val legacyGeneralPrefs = PreferenceManager.getDefaultSharedPreferences(context).all
        val hasLegacyGeneralPrefs = legacyGeneralPrefs.isNotEmpty()
            && (legacyGeneralPrefs.size > 1 || !legacyGeneralPrefs.containsKey("bg_startup_tracing"))

I thought about doing a PR for this but it doesn't seem to happen even on the A13 unless I'm launching from Android Studio so maybe it doesn't need to be addressed. I asked about the pref on Stackoverflow and CommonsWare said it could be related to WebView.

I printed out the actual time for each benchmark:
Downloading form with http cache: 88
Updating form with http cache: 176
Loading form first time: 4
Loading form second time: 2
Filtering select: 5

That means the current assertions won't pass with the target device. I think we can merge this and then follow up with targeted improvements? Or make those here, I don't have a strong preference!

import org.odk.collect.strings.R

/**
* Benchmarks the performance of entity follow up forms. [PROJECT_URL] should be set to a project
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to have a demo server for such test forms so that we don't have to configure anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! I think we're potentially planning that for a beta as well. Let's leave this as-is for the moment, but I like the idea of that being how it works later.


.clickOK(MainMenuPage())
.clickGetBlankForm()
.benchmark("Updating form with http cache", stopwatch) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's the same form without any updates it's not really updating, right? Downloading will be skipped so what takes time here is comparing the differences. Am I right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it should really be "Checking for updates when there are none" or something. I'll rename.

assertThat(stopwatch.getTime("Updating form with http cache"), lessThan(90))
assertThat(stopwatch.getTime("Loading form first time"), lessThan(5))
assertThat(stopwatch.getTime("Loading form second time"), lessThan(5))
assertThat(stopwatch.getTime("Filtering select"), lessThan(5))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can add const for those keys as they are used here and above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the way we benchmark/assert so that they only need to be referenced once instead.

@seadowg
Copy link
Member Author

seadowg commented Aug 20, 2024

That means the current assertions won't pass with the target device. I think we can merge this and then follow up with targeted improvements? Or make those here, I don't have a strong preference!

Yeah, let's follow up as this PR is blocking other changes.

@seadowg seadowg merged commit 30f759c into getodk:master Aug 20, 2024
6 checks passed
@seadowg seadowg deleted the benchmark branch August 20, 2024 14:27
@WKobus
Copy link

WKobus commented Aug 21, 2024

Tested with Success!

Verified on device with Android 14, 10

Verified cases:

  • Regression check on entity forms
  • Performance on 100k entity forms

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