-
Notifications
You must be signed in to change notification settings - Fork 62
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
Merged
ellykits
merged 4 commits into
fix-register-filters
from
3440-batch-fetch-search-count-all
Sep 19, 2024
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
22a908f
Fetch search results in batches for when loading all
LZRS 5022fa8
Fix infinite loop for mocks with FhirEngine#search in tests
LZRS 8f4e11c
Add comparable FhirEngine#search vs batchedSearch integration tests
LZRS 5966fe5
Merge remote-tracking branch 'origin/fix-register-filters' into 3440-…
LZRS File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
101 changes: 101 additions & 0 deletions
101
...idTest/java/org/smartregister/fhircore/engine/util/extension/FhirEngineExtensionKtTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| /* | ||
| * Copyright 2021-2024 Ona Systems, Inc | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.smartregister.fhircore.engine.util.extension | ||
|
|
||
| import android.content.Context | ||
| import androidx.test.core.app.ApplicationProvider | ||
| import androidx.test.filters.MediumTest | ||
| import com.google.android.fhir.FhirEngine | ||
| import com.google.android.fhir.FhirEngineConfiguration | ||
| import com.google.android.fhir.FhirEngineProvider | ||
| import com.google.android.fhir.search.search | ||
| import kotlinx.coroutines.launch | ||
| import kotlinx.coroutines.runBlocking | ||
| import org.hl7.fhir.r4.model.Patient | ||
| import org.hl7.fhir.r4.model.Questionnaire | ||
| import org.hl7.fhir.r4.model.Resource | ||
| import org.hl7.fhir.r4.model.ResourceType | ||
| import org.junit.After | ||
| import org.junit.Assert | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
|
|
||
| @MediumTest | ||
| class FhirEngineExtensionKtTest { | ||
|
|
||
| private val context = ApplicationProvider.getApplicationContext<Context>() | ||
| private lateinit var fhirEngine: FhirEngine | ||
|
|
||
| @Before | ||
| fun setUp() { | ||
| FhirEngineProvider.init(FhirEngineConfiguration(testMode = true)) | ||
| fhirEngine = FhirEngineProvider.getInstance(context) | ||
|
|
||
| val patients = (0..1000).map { Patient().apply { id = "test-patient-$it" } } | ||
| val questionnaires = (0..3).map { Questionnaire().apply { id = "test-questionnaire-$it" } } | ||
| runBlocking { fhirEngine.create(*patients.toTypedArray(), *questionnaires.toTypedArray()) } | ||
| } | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| runBlocking { fhirEngine.clearDatabase() } | ||
| FhirEngineProvider.cleanup() | ||
| } | ||
|
|
||
| @Test | ||
| fun test_search_time_searches_sequentially_and_short_running_query_waits() { | ||
| val fetchedResources = mutableListOf<Resource>() | ||
| runBlocking { | ||
| launch { | ||
| val patients = fhirEngine.search<Patient> {}.map { it.resource } | ||
| fetchedResources += patients | ||
| } | ||
|
|
||
| launch { | ||
| val questionnaires = fhirEngine.search<Questionnaire> {}.map { it.resource } | ||
| fetchedResources += questionnaires | ||
| } | ||
| } | ||
| val indexOfResultOfShortQuery = | ||
| fetchedResources.indexOfFirst { it.resourceType == ResourceType.Questionnaire } | ||
| val indexOfResultOfLongQuery = | ||
| fetchedResources.indexOfFirst { it.resourceType == ResourceType.Patient } | ||
| Assert.assertTrue(indexOfResultOfShortQuery > indexOfResultOfLongQuery) | ||
| } | ||
|
|
||
| @Test | ||
| fun test_batchedSearch_returns_short_running_query_and_long_running_does_not_block() { | ||
| val fetchedResources = mutableListOf<Resource>() | ||
| runBlocking { | ||
| launch { | ||
| val patients = fhirEngine.batchedSearch<Patient> {}.map { it.resource } | ||
| fetchedResources += patients | ||
| } | ||
|
|
||
| launch { | ||
| val questionnaires = fhirEngine.search<Questionnaire> {} | ||
| fetchedResources + questionnaires | ||
| } | ||
| } | ||
|
|
||
| val indexOfResultOfShortQuery = | ||
| fetchedResources.indexOfFirst { it.resourceType == ResourceType.Questionnaire } | ||
| val indexOfResultOfLongQuery = | ||
| fetchedResources.indexOfFirst { it.resourceType == ResourceType.Patient } | ||
| Assert.assertTrue(indexOfResultOfShortQuery < indexOfResultOfLongQuery) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OFFSETrequires the database to skip over recordsbefore 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.
cc @ndegwamartin @pld
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: