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

Impropve transaction Performance #2717

Merged
merged 13 commits into from
Jun 14, 2021
Merged

Conversation

jamesagnew
Copy link
Collaborator

@jamesagnew jamesagnew commented Jun 9, 2021

At the start of processing a transaction, we now gather up all of the forced IDs, and resolve them in a single SQL statement instead of letting each entry in the bundle handle the resolution one-by-one
Similarly, if there are conditional URLs (aka Match URLs) for conditional creates or conditional updates, and those URLs have a single parameter and that parameter is a token parameter (this seems hyper specific but it's the most common scenario for conditional operations if you think about it) we pre-resolve these in a single SQL statement for all of the URLs too
We then pre-fetch the index table collections for all of these pre-fetched resources in a small set of SQL statements instead of letting each inner operation handle that.

In my tests using a set of sample data, this takes a 7-resource transaction bundle down from executing about 33 SQL statements to executing 13.

@lgtm-com
Copy link

lgtm-com bot commented Jun 9, 2021

This pull request introduces 1 alert when merging 3f36f0a into 3863e82 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Jun 9, 2021

This pull request introduces 1 alert when merging a5db326 into 3863e82 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Jun 9, 2021

This pull request introduces 1 alert when merging 994bd3b into 3863e82 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Jun 9, 2021

This pull request introduces 1 alert when merging 6c92cf2 into 3863e82 - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@codecov
Copy link

codecov bot commented Jun 14, 2021

Codecov Report

Merging #2717 (b699aa2) into master (3863e82) will increase coverage by 0.03%.
The diff coverage is 92.64%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2717      +/-   ##
============================================
+ Coverage     82.52%   82.55%   +0.03%     
- Complexity    19079    19169      +90     
============================================
  Files          1280     1281       +1     
  Lines         68458    68695     +237     
  Branches      10482    10536      +54     
============================================
+ Hits          56492    56714     +222     
- Misses         7915     7918       +3     
- Partials       4051     4063      +12     
Impacted Files Coverage Δ
...fhir/jpa/dao/FhirResourceDaoSubscriptionDstu2.java 72.22% <0.00%> (ø)
...pa/dao/dstu3/FhirResourceDaoSubscriptionDstu3.java 72.22% <0.00%> (ø)
...ca/uhn/fhir/jpa/sp/SearchParamPresenceSvcImpl.java 100.00% <ø> (ø)
...e/src/main/java/ca/uhn/fhir/jpa/util/SqlQuery.java 88.57% <ø> (-0.62%) ⬇️
.../model/entity/ResourceIndexedSearchParamToken.java 94.28% <ø> (ø)
...main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java 88.59% <88.09%> (-1.72%) ⬇️
...ava/ca/uhn/fhir/jpa/dao/index/IdHelperService.java 82.12% <92.00%> (+0.34%) ⬆️
...java/ca/uhn/fhir/jpa/dao/TransactionProcessor.java 91.01% <92.30%> (+7.68%) ⬆️
...ir/rest/api/server/storage/TransactionDetails.java 91.93% <92.85%> (+4.18%) ⬆️
...a/ca/uhn/fhir/jpa/dao/MatchResourceUrlService.java 90.74% <93.33%> (-3.38%) ⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3863e82...b699aa2. Read the comment docs.

@jamesagnew jamesagnew merged commit b934abb into master Jun 14, 2021
@jamesagnew jamesagnew deleted the ja_20210608_transaction_performance branch June 14, 2021 17:08
List<List<IQueryParameterType>> andList = values.iterator().next();
IQueryParameterType param = andList.get(0).get(0);

if (param instanceof TokenParam) {
Copy link
Contributor

@tuomoa tuomoa May 19, 2022

Choose a reason for hiding this comment

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

What about other types of params such as UriParam?

Because we are not handling UriParam here, and on line 278 the requestUrl is marked as not found later on in the resourceDao update search will be skipped and duplicate resource will be created for a conditional url such as CodeSystem?url=http://example.com/something.

Edit: Opened up #3625

for (MatchUrlToResolve nextSearchParameterMap : searchParameterMapsToResolve) {
// No matches
if (!nextSearchParameterMap.myResolved) {
theTransactionDetails.addResolvedMatchUrl(nextSearchParameterMap.myRequestUrl, TransactionDetails.NOT_FOUND);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not mark matchUrl as not found if it was not processed (if it wasn't TokenParam).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants