Revise saveConcept method to use save() instead of saveAndFlush() #575

Merged
merged 1 commit into from Mar 17, 2017

Conversation

Projects
None yet
4 participants
@joelsch

joelsch commented Mar 1, 2017

Closes #574

Revise saveConcept method to use myConceptDao.save() instead of myConceptDao.saveAndFlush(), to avoid excessive CPU utilization when persisting a large CodeSystem (e.g. one with 399837 concepts).

Commentary: It seems the Hibernate flush operation contains code which performs an O(N^2) operation on the cache, and saveAndFlush causes that to be performed N times. Not good when N is 399837.

I think this change is reasonably safe, because saveConcept is only used within a transaction context. However, this may also benefit from a review by someone more familiar with HAPI's inner workings.

Revise saveConcept method to use myConceptDao.save instead of myConce…
…ptDao.saveAndFlush (to avoid overloading CPU with flush computations when persisting large codesystems, e.g. one with 399837 concepts).
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 1, 2017

Coverage Status

Coverage remained the same at 81.75% when pulling 2f1662b on joelsch:large-codesystem-persistence-fix into 0d06627 on jamesagnew:master.

Coverage Status

Coverage remained the same at 81.75% when pulling 2f1662b on joelsch:large-codesystem-persistence-fix into 0d06627 on jamesagnew:master.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Mar 1, 2017

Owner

Does your great big transaction bundle save correctly with this change?

I think the original intent of the constant flushing was to avoid having the JPA session keeping all of the entities in memory as it saves. Presumably that was an unneccesary "optimization" if this is working with your proposed change. :)

Owner

jamesagnew commented Mar 1, 2017

Does your great big transaction bundle save correctly with this change?

I think the original intent of the constant flushing was to avoid having the JPA session keeping all of the entities in memory as it saves. Presumably that was an unneccesary "optimization" if this is working with your proposed change. :)

@joelsch

This comment has been minimized.

Show comment
Hide comment
@joelsch

joelsch Mar 1, 2017

Yes, my great big transaction bundle saves correctly with this change. Here's an abbreviated log file excerpt ...

2017-03-01 05:07:40.647 [http-nio-8080-exec-8] INFO  c.u.f.j.dao.dstu3.FhirSystemDaoDstu3 [FhirSystemDaoDstu3.java:281] Beginning Transaction with 2 resources
2017-03-01 05:07:40.648 [http-nio-8080-exec-8] INFO  c.u.f.j.dao.dstu3.FhirSystemDaoDstu3 [FhirSystemDaoDstu3.java:333] Processed 0 non-GET entries out of 2
2017-03-01 05:07:45.334 [http-nio-8080-exec-8] INFO  c.u.f.j.d.d.FhirResourceDaoCodeSystemDstu3 [FhirResourceDaoCodeSystemDstu3.java:204] CodeSystem CodeSystem/1/_history/0 has a status of complete, going to store concepts in terminology tables
2017-03-01 05:07:46.449 [http-nio-8080-exec-8] INFO  c.u.f.j.d.d.FhirResourceDaoCodeSystemDstu3 [FhirResourceDaoCodeSystemDstu3.java:216] Code system has 399837 concepts
2017-03-01 05:07:46.450 [http-nio-8080-exec-8] INFO  c.u.f.j.term.BaseHapiTerminologySvc [BaseHapiTerminologySvc.java:470] Storing code system
...
2017-03-01 05:34:37.111 [http-nio-8080-exec-8] INFO  c.u.f.j.dao.dstu3.FhirSystemDaoDstu3 [FhirSystemDaoDstu3.java:602] Transaction completed in 1616464ms
2017-03-01 05:40:02.044 [scheduledExecutorService-5] INFO  c.u.f.j.term.BaseHapiTerminologySvc [BaseHapiTerminologySvc.java:427] Saving 2000 deferred concepts...
...
2017-03-01 05:49:04.738 [scheduledExecutorService-2] INFO  c.u.f.j.term.BaseHapiTerminologySvc [BaseHapiTerminologySvc.java:458] All deferred concepts and relationships have now been synchronized to the database

joelsch commented Mar 1, 2017

Yes, my great big transaction bundle saves correctly with this change. Here's an abbreviated log file excerpt ...

2017-03-01 05:07:40.647 [http-nio-8080-exec-8] INFO  c.u.f.j.dao.dstu3.FhirSystemDaoDstu3 [FhirSystemDaoDstu3.java:281] Beginning Transaction with 2 resources
2017-03-01 05:07:40.648 [http-nio-8080-exec-8] INFO  c.u.f.j.dao.dstu3.FhirSystemDaoDstu3 [FhirSystemDaoDstu3.java:333] Processed 0 non-GET entries out of 2
2017-03-01 05:07:45.334 [http-nio-8080-exec-8] INFO  c.u.f.j.d.d.FhirResourceDaoCodeSystemDstu3 [FhirResourceDaoCodeSystemDstu3.java:204] CodeSystem CodeSystem/1/_history/0 has a status of complete, going to store concepts in terminology tables
2017-03-01 05:07:46.449 [http-nio-8080-exec-8] INFO  c.u.f.j.d.d.FhirResourceDaoCodeSystemDstu3 [FhirResourceDaoCodeSystemDstu3.java:216] Code system has 399837 concepts
2017-03-01 05:07:46.450 [http-nio-8080-exec-8] INFO  c.u.f.j.term.BaseHapiTerminologySvc [BaseHapiTerminologySvc.java:470] Storing code system
...
2017-03-01 05:34:37.111 [http-nio-8080-exec-8] INFO  c.u.f.j.dao.dstu3.FhirSystemDaoDstu3 [FhirSystemDaoDstu3.java:602] Transaction completed in 1616464ms
2017-03-01 05:40:02.044 [scheduledExecutorService-5] INFO  c.u.f.j.term.BaseHapiTerminologySvc [BaseHapiTerminologySvc.java:427] Saving 2000 deferred concepts...
...
2017-03-01 05:49:04.738 [scheduledExecutorService-2] INFO  c.u.f.j.term.BaseHapiTerminologySvc [BaseHapiTerminologySvc.java:458] All deferred concepts and relationships have now been synchronized to the database
@joelsch

This comment has been minimized.

Show comment
Hide comment
@joelsch

joelsch Mar 2, 2017

Additionally, to process the large bundle, I increased the Java heap size to 2048m. 1536m might also work.

joelsch commented Mar 2, 2017

Additionally, to process the large bundle, I increased the Java heap size to 2048m. 1536m might also work.

jamesagnew added a commit that referenced this pull request Mar 17, 2017

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Mar 17, 2017

Owner

Fantastic, merging this now! Thanks again!

Owner

jamesagnew commented Mar 17, 2017

Fantastic, merging this now! Thanks again!

@jamesagnew jamesagnew merged commit 899ed25 into jamesagnew:master Mar 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 81.75%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment