Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Improve risklevel result data access performance (DEV) #1953

Merged

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Dec 18, 2020

Got pinged by testers about the app taking longer to load, further investigation showed several issues that this PR fixes. The underlying issues are not present (or only to a lot lesser degree) on production:

  • Exposure Window data was being explicitly logged in tester builds, due to much larger scan instance counts, the logging slowed down data emission by Flow that printed the log output.

    • I've changed the data class to exclude scan instance output via toString, reducing it to only show the item count.
  • RiskLevel result history size is much larger for the tester builds, and tester builds store the exposure window data. We only had a single way of accessing the stored risk result which meant that the access triggered retrieval of all results (and required mapping matching exposure windows to each result).

    • There is a new latestRiskLevelResults that uses SQL queries to prefilter the data (faster than in kotlin) a return only the latest two results (which is what the UI is mostly interested in, except for test screens)
  • Multiple data access was happening, where we could have reused the results, switching from home to the risk details screen, caused 2 sepperate retrieval events.

    • We are now using kotlin stateIn to share the flow subscription. Currently it's only WhileSubscribed with a replay value of 0ms but due to async viewmodel teardown when leaving the home screen this is already enough to prevent double flow subscription.

Testing

Also do a lookup in Android Studio on where each Flow is accessed from and whether it's the correct data being supplied.

@d4rken d4rken added bug Something isn't working enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers labels Dec 18, 2020
@d4rken d4rken added this to the 1.11.0 milestone Dec 18, 2020
@d4rken d4rken requested a review from a team December 18, 2020 15:19
BMItr
BMItr previously approved these changes Dec 18, 2020
Copy link
Contributor

@BMItr BMItr left a comment

Choose a reason for hiding this comment

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

I'm unsure whether these changes will be sufficent for the mentioned problem.
Issue should be kept in mind. @kolyaopahle
Code lgtm.

@BMItr BMItr self-assigned this Dec 18, 2020
@d4rken
Copy link
Member Author

d4rken commented Dec 18, 2020

I'm unsure whether these changes will be sufficent for the mentioned problem.

I concur that we should keep an 👁️ on this.

I added some debug code that multiplied the processed windows by 100 and it was better, there may be issues where the amount of scan instances is high. The next step would probably be to created combined SQL queries, this was problematic at first because we only care about ExposureWindows on tester builds 🤔

There may be further optimizations by increasing the "replay duration" i.e. how long the timeout is during which the last value will be replayed, but that's a different topic, where we have to check if there are certain constellations where the data is accessed via .first() and replaying values could cause subtle bugs.

@d4rken d4rken marked this pull request as draft December 19, 2020 10:07
@ralfgehrer ralfgehrer self-assigned this Dec 21, 2020
ralfgehrer
ralfgehrer previously approved these changes Dec 21, 2020
Copy link
Contributor

@ralfgehrer ralfgehrer left a comment

Choose a reason for hiding this comment

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

lgtm

@d4rken d4rken dismissed stale reviews from ralfgehrer and BMItr via 14255e6 December 21, 2020 11:36
@d4rken d4rken marked this pull request as ready for review December 21, 2020 11:53
ralfgehrer
ralfgehrer previously approved these changes Dec 22, 2020
…erformance

# Conflicts:
#	Corona-Warn-App/src/deviceForTesters/java/de/rki/coronawarnapp/test/risklevel/ui/TestRiskLevelCalculationFragmentCWAViewModel.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/card/TracingCardStateProvider.kt
#	Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/tracing/details/TracingDetailsStateProvider.kt
ralfgehrer
ralfgehrer previously approved these changes Dec 23, 2020
@BMItr
Copy link
Contributor

BMItr commented Dec 23, 2020

looking good so far.
current branch doesn't allow me to run RiskResultDatabaseTest (due some other testproblems (HomeFragmentTest?))
could you check this at last?

@d4rken
Copy link
Member Author

d4rken commented Dec 23, 2020

current branch doesn't allow me to run RiskResultDatabaseTest (due some other testproblems (HomeFragmentTest?))
could you check this at last?

Done. The HomeFragment test does not pass since the big refactoring of the home screen, but the other tests can be run again.

Copy link
Contributor

@BMItr BMItr left a comment

Choose a reason for hiding this comment

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

👌

@sonarcloud
Copy link

sonarcloud bot commented Dec 23, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

79.3% 79.3% Coverage
0.0% 0.0% Duplication

@ralfgehrer ralfgehrer merged commit 665b6f2 into release/1.11.x Dec 23, 2020
@ralfgehrer ralfgehrer deleted the fix/risklevel-result-data-access-performance branch December 23, 2020 15:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants