-
Notifications
You must be signed in to change notification settings - Fork 59
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
Batch searching when list might be large or when count is not defined #3456
Batch searching when list might be large or when count is not defined #3456
Conversation
val result = mutableListOf<SearchResult<R>>() | ||
var offset = search.from ?: 0 | ||
val pageCount = 100 | ||
do { | ||
search.from = offset | ||
search.count = pageCount | ||
val searchResults = this.search<R>(search) | ||
result += searchResults | ||
offset += searchResults.size | ||
} while (searchResults.size == pageCount) | ||
|
||
result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @LZRS.
I have a few questions and comments on this implementation.
The decision to load this data in batches sounds reasonable, however, based on this implementation I believe the gain would be negligible or there would be no gain at all. Technically, the data will still be cached in memory inside the loop. This differs from the approach where we would load and process the data in batches. Data processing should happen after fetching each batch to improve the app responsiveness, otherwise, there are some drawbacks with batching including:
-
Each batch fetch requires a new database query, introducing overhead. The overhead of executing smaller queries may outweigh the benefit of lower memory.
-
Using
OFFSET
requires the database to skip over records
before returning the batch. A larger offset would require scanning and discarding many rows thus if indexing is not done properly, this will impact fetching data in batches.
@LZRS Would it be possible to measure the performance impact introduced by this implementation for comparison?
The paging3 library loads data in batches and allows for data processing with each fetch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, generally, I think the performance gains would probably be negligible (in one of our cases, a difference of 20ms) especially when assuming only our query of interest would be running a single time. I think the significance of these changes would come about when we have multiple queries from different coroutines/threads.
Assuming for example we have a background long running operation accessing the db, and in the UI someone goes to a page that to render also requires a db fetch. Instead of waiting for the long running background operation to finish, the db fetch for the UI page can get queued in between the batches for the long running background. Looking to also add integration tests on the same
The changes here would ensure that no single query(in batch) would take more than a couple of milliseconds, a long running query would still take long to be loaded (as it's split and cached in memory) but it would allow space for other queries to also get run.
I also agree that the best case would be data processing after fetching each batch although that would be quite tasking to implement, I feel.
In terms of MalawiCore, it improved the app's performance from here to here.
We'll working on similar tests for FhirCore.
Use of paging3 to load would be the recommended approach but it would also need approval from the Google's sdk team that would also take time, I think
Note:
Room will only perform at most one transaction at a time, additional transactions are queued and executed on a first come, first serve order.
856f86c
to
4de7da5
Compare
29b1043
to
6ee690d
Compare
...droidTest/java/org/smartregister/fhircore/engine/util/extension/FhirEngineExtensionKtTest.kt
Outdated
Show resolved
Hide resolved
6ee690d
to
345552a
Compare
…batch-fetch-search-count-all
345552a
to
5966fe5
Compare
* Refactor register filter with REL tags Signed-off-by: Elly Kitoto <[email protected]> * Update default pageSize to 15 Signed-off-by: Elly Kitoto <[email protected]> * Fix loading locations on map Signed-off-by: Elly Kitoto <[email protected]> * Refactor data structure used on base resource search results Signed-off-by: Elly Kitoto <[email protected]> * Refactor code Signed-off-by: Elly Kitoto <[email protected]> * Delete unnecessary code Signed-off-by: Elly Kitoto <[email protected]> * Refactor retrieving related resources Signed-off-by: Elly Kitoto <[email protected]> * Optimize data structures and perform parallel processing Signed-off-by: Elly Kitoto <[email protected]> * Use recent version of rules engine library Signed-off-by: Elly Kitoto <[email protected]> * Refactor implementation for decoding image resources to bitmap Signed-off-by: Elly Kitoto <[email protected]> * Fix related resource count on register Signed-off-by: Elly Kitoto <[email protected]> * Fix loading related resources This fix ensures all the nested related resources are loaded too. Signed-off-by: Elly Kitoto <[email protected]> * Batch related resource queries Signed-off-by: Elly Kitoto <[email protected]> * Map resources to RepositoryResourceData with async map Signed-off-by: Elly Kitoto <[email protected]> * Make infinite scroll the default register behavior Signed-off-by: Elly Kitoto <[email protected]> * Disable automatic intialization of emoji2 A lot of memory was used in heap during the allocation. Emojis are not used in the app so intializing them automatically is unnecessary. Signed-off-by: Elly Kitoto <[email protected]> * ⬆️ Update the map box and kujaku versions * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> * Update tests for displaying images (#3506) * Refactor load images tests for different views. Signed-off-by: Lentumunai-Mark <[email protected]> * Remove unutilized imports. Signed-off-by: Lentumunai-Mark <[email protected]> * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> Signed-off-by: Lentumunai-Mark <[email protected]> * Refactor load images tests for different views. Signed-off-by: Lentumunai-Mark <[email protected]> * Resolve conflicts. Signed-off-by: Lentumunai-Mark <[email protected]> --------- Signed-off-by: Lentumunai-Mark <[email protected]> Signed-off-by: Elly Kitoto <[email protected]> * Refactor navigation to GeowidgetLauncher workflow Signed-off-by: Elly Kitoto <[email protected]> * Load map data in batches (#3511) * Load map data in batches Signed-off-by: Elly Kitoto <[email protected]> * Update observer Signed-off-by: Elly Kitoto <[email protected]> * Deactivate infinite scroll by default Signed-off-by: Elly Kitoto <[email protected]> --------- Signed-off-by: Elly Kitoto <[email protected]> * Fix failing tests Signed-off-by: Elly Kitoto <[email protected]> * Batch searching when list might be large or when count is not defined (#3456) * Fetch search results in batches for when loading all * Fix infinite loop for mocks with FhirEngine#search in tests * Add comparable FhirEngine#search vs batchedSearch integration tests * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> * Fix Geowidget tests Signed-off-by: Elly Kitoto <[email protected]> * Fix spotlessCheck Signed-off-by: Elly Kitoto <[email protected]> * Add missing import Signed-off-by: Elly Kitoto <[email protected]> * Fix rules execution before map render Signed-off-by: Elly Kitoto <[email protected]> * - Update android manifest to use exported values * Prevent leaking map features via viewmodel Signed-off-by: Elly Kitoto <[email protected]> * Format code Signed-off-by: Elly Kitoto <[email protected]> * Only search map features via keyboard action We need to reload all features when the search term is reset Signed-off-by: Elly Kitoto <[email protected]> --------- Signed-off-by: Elly Kitoto <[email protected]> Signed-off-by: Lentumunai-Mark <[email protected]> Co-authored-by: Benjamin Mwalimu <[email protected]> Co-authored-by: Lentumunai Mark <[email protected]> Co-authored-by: L≡ZRS <[email protected]>
* Refactor register filter with REL tags Signed-off-by: Elly Kitoto <[email protected]> * Update default pageSize to 15 Signed-off-by: Elly Kitoto <[email protected]> * Fix loading locations on map Signed-off-by: Elly Kitoto <[email protected]> * Refactor data structure used on base resource search results Signed-off-by: Elly Kitoto <[email protected]> * Refactor code Signed-off-by: Elly Kitoto <[email protected]> * Delete unnecessary code Signed-off-by: Elly Kitoto <[email protected]> * Refactor retrieving related resources Signed-off-by: Elly Kitoto <[email protected]> * Optimize data structures and perform parallel processing Signed-off-by: Elly Kitoto <[email protected]> * Use recent version of rules engine library Signed-off-by: Elly Kitoto <[email protected]> * Refactor implementation for decoding image resources to bitmap Signed-off-by: Elly Kitoto <[email protected]> * Fix related resource count on register Signed-off-by: Elly Kitoto <[email protected]> * Fix loading related resources This fix ensures all the nested related resources are loaded too. Signed-off-by: Elly Kitoto <[email protected]> * Batch related resource queries Signed-off-by: Elly Kitoto <[email protected]> * Map resources to RepositoryResourceData with async map Signed-off-by: Elly Kitoto <[email protected]> * Make infinite scroll the default register behavior Signed-off-by: Elly Kitoto <[email protected]> * Disable automatic intialization of emoji2 A lot of memory was used in heap during the allocation. Emojis are not used in the app so intializing them automatically is unnecessary. Signed-off-by: Elly Kitoto <[email protected]> * ⬆️ Update the map box and kujaku versions * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> * Update tests for displaying images (#3506) * Refactor load images tests for different views. Signed-off-by: Lentumunai-Mark <[email protected]> * Remove unutilized imports. Signed-off-by: Lentumunai-Mark <[email protected]> * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> Signed-off-by: Lentumunai-Mark <[email protected]> * Refactor load images tests for different views. Signed-off-by: Lentumunai-Mark <[email protected]> * Resolve conflicts. Signed-off-by: Lentumunai-Mark <[email protected]> --------- Signed-off-by: Lentumunai-Mark <[email protected]> Signed-off-by: Elly Kitoto <[email protected]> * Refactor navigation to GeowidgetLauncher workflow Signed-off-by: Elly Kitoto <[email protected]> * Load map data in batches (#3511) * Load map data in batches Signed-off-by: Elly Kitoto <[email protected]> * Update observer Signed-off-by: Elly Kitoto <[email protected]> * Deactivate infinite scroll by default Signed-off-by: Elly Kitoto <[email protected]> --------- Signed-off-by: Elly Kitoto <[email protected]> * Fix failing tests Signed-off-by: Elly Kitoto <[email protected]> * Batch searching when list might be large or when count is not defined (#3456) * Fetch search results in batches for when loading all * Fix infinite loop for mocks with FhirEngine#search in tests * Add comparable FhirEngine#search vs batchedSearch integration tests * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> * Fix Geowidget tests Signed-off-by: Elly Kitoto <[email protected]> * Fix spotlessCheck Signed-off-by: Elly Kitoto <[email protected]> * Add missing import Signed-off-by: Elly Kitoto <[email protected]> * Fix rules execution before map render Signed-off-by: Elly Kitoto <[email protected]> * - Update android manifest to use exported values * Prevent leaking map features via viewmodel Signed-off-by: Elly Kitoto <[email protected]> * Format code Signed-off-by: Elly Kitoto <[email protected]> * Only search map features via keyboard action We need to reload all features when the search term is reset Signed-off-by: Elly Kitoto <[email protected]> --------- Signed-off-by: Elly Kitoto <[email protected]> Signed-off-by: Lentumunai-Mark <[email protected]> Co-authored-by: Benjamin Mwalimu <[email protected]> Co-authored-by: Lentumunai Mark <[email protected]> Co-authored-by: L≡ZRS <[email protected]>
* Refactor register filter with REL tags Signed-off-by: Elly Kitoto <[email protected]> * Update default pageSize to 15 Signed-off-by: Elly Kitoto <[email protected]> * Fix loading locations on map Signed-off-by: Elly Kitoto <[email protected]> * Refactor data structure used on base resource search results Signed-off-by: Elly Kitoto <[email protected]> * Refactor code Signed-off-by: Elly Kitoto <[email protected]> * Delete unnecessary code Signed-off-by: Elly Kitoto <[email protected]> * Refactor retrieving related resources Signed-off-by: Elly Kitoto <[email protected]> * Optimize data structures and perform parallel processing Signed-off-by: Elly Kitoto <[email protected]> * Use recent version of rules engine library Signed-off-by: Elly Kitoto <[email protected]> * Refactor implementation for decoding image resources to bitmap Signed-off-by: Elly Kitoto <[email protected]> * Fix related resource count on register Signed-off-by: Elly Kitoto <[email protected]> * Fix loading related resources This fix ensures all the nested related resources are loaded too. Signed-off-by: Elly Kitoto <[email protected]> * Batch related resource queries Signed-off-by: Elly Kitoto <[email protected]> * Map resources to RepositoryResourceData with async map Signed-off-by: Elly Kitoto <[email protected]> * Make infinite scroll the default register behavior Signed-off-by: Elly Kitoto <[email protected]> * Disable automatic intialization of emoji2 A lot of memory was used in heap during the allocation. Emojis are not used in the app so intializing them automatically is unnecessary. Signed-off-by: Elly Kitoto <[email protected]> * ⬆️ Update the map box and kujaku versions * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> * Update tests for displaying images (opensrp#3506) * Refactor load images tests for different views. Signed-off-by: Lentumunai-Mark <[email protected]> * Remove unutilized imports. Signed-off-by: Lentumunai-Mark <[email protected]> * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> Signed-off-by: Lentumunai-Mark <[email protected]> * Refactor load images tests for different views. Signed-off-by: Lentumunai-Mark <[email protected]> * Resolve conflicts. Signed-off-by: Lentumunai-Mark <[email protected]> --------- Signed-off-by: Lentumunai-Mark <[email protected]> Signed-off-by: Elly Kitoto <[email protected]> * Refactor navigation to GeowidgetLauncher workflow Signed-off-by: Elly Kitoto <[email protected]> * Load map data in batches (opensrp#3511) * Load map data in batches Signed-off-by: Elly Kitoto <[email protected]> * Update observer Signed-off-by: Elly Kitoto <[email protected]> * Deactivate infinite scroll by default Signed-off-by: Elly Kitoto <[email protected]> --------- Signed-off-by: Elly Kitoto <[email protected]> * Fix failing tests Signed-off-by: Elly Kitoto <[email protected]> * Batch searching when list might be large or when count is not defined (opensrp#3456) * Fetch search results in batches for when loading all * Fix infinite loop for mocks with FhirEngine#search in tests * Add comparable FhirEngine#search vs batchedSearch integration tests * Run spotlessApply Signed-off-by: Elly Kitoto <[email protected]> * Fix Geowidget tests Signed-off-by: Elly Kitoto <[email protected]> * Fix spotlessCheck Signed-off-by: Elly Kitoto <[email protected]> * Add missing import Signed-off-by: Elly Kitoto <[email protected]> * Fix rules execution before map render Signed-off-by: Elly Kitoto <[email protected]> * - Update android manifest to use exported values * Prevent leaking map features via viewmodel Signed-off-by: Elly Kitoto <[email protected]> * Format code Signed-off-by: Elly Kitoto <[email protected]> * Only search map features via keyboard action We need to reload all features when the search term is reset Signed-off-by: Elly Kitoto <[email protected]> --------- Signed-off-by: Elly Kitoto <[email protected]> Signed-off-by: Lentumunai-Mark <[email protected]> Co-authored-by: Benjamin Mwalimu <[email protected]> Co-authored-by: Lentumunai Mark <[email protected]> Co-authored-by: L≡ZRS <[email protected]>
IMPORTANT: Where possible all PRs must be linked to a Github issue
Fixes #3440
Engineer Checklist
strings.xml
file./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the project's style guideCode Reviewer Checklist
strings.xml
file