-
Notifications
You must be signed in to change notification settings - Fork 293
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
Implement Parallelized map and optimize Database search API #2669
Conversation
- Optimize Database search API
0eb0187
to
2fc407c
Compare
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.
The change looks great!
Additionally, could you provide some performance comparison between the old and new code? That will be cool to know
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.
great work thanks @ndegwamartin!
FORK - With unmerged PR #9 - WUP #13 SDK - WUP google#2178 - WUP google#2650 - WUP google#2663 PERF - WUP google#2669 - WUP google#2565 - WUP google#2561 - WUP google#2535
To summarised our discussion yesterday, I think there's still work to be done in this PR - @ndegwamartin to investigate thread pool etc. Pls comment when this is ready for next round of review - @FikriMilano @aditya-07 @yigit @stevenckngaa @vorburger @kevinmost pls also take a look at this. |
Device: Physical, Samsung Galaxy Active Tab 2 Optimization: None Run 1
Run 2
Optimization: Using async with parent context (usually Run 1
Run 2
Optimization: Using async with Run 1
Run 2
Note - The tests were carried out in a QA test environment. In the real world Patients would be more than Groups (i.e. Patients = ~10 x No. of Groups ) and Tasks would be even more than Patients (i.e Tasks = ~30 x No. of Patients) |
Full specs of the device:
|
engine/src/main/java/com/google/android/fhir/db/impl/DatabaseImpl.kt
Outdated
Show resolved
Hide resolved
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.
Looks good!
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.
What happens when you don't parallelize the parsing but just run the parsing in the Default Dispatcher ?
A new function:-
fun IParser.parseResourceInCPU(resourceString: String) =
withContext(Dispatchers.Default) { parseResource(resourceString) }
and replace all parseResource with this function.
I am surprised that parallelizing (second optimization) did not improve the search api performance. If parallelizing is not improving then do we really need it ?
Device: Physical, Samsung Galaxy Active Tab 2 Optimization: Using IParser.parseResourceInCPU() with context Run 1
Run 2
cc @MJ1998 |
So its only when we do both - parallelize and use Default dispatcher - we see performance improvement, correct ? |
Correct. |
Okay so I think using Dispatchers.IO for CPU-intensive tasks might be less efficient due to thread type mismatch. |
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 @ndegwamartin for all the work! great results!
FORK - With unmerged PR #9 - WUP #13 SDK - WUP google#2178 - WUP google#2650 - WUP google#2663 PERF - WUP google#2669 - WUP google#2565 - WUP google#2561 - WUP google#2535
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.
LGTM.
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2668
Description
Optimizes the DatabaseImpl search API's serialized FHIR Resource to HAPI FHIR Structure mapping block by introducing a parallelized implementation that uses async coroutines for each mapping iteration.
Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Enhancement
Screenshots (if applicable)
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.