perf: project program stage data elements in preheat - #24773
Draft
teleivo wants to merge 5 commits into
Draft
Conversation
Mapping ProgramStage.programStageDataElements initialized one DataElement entity per element, thousands on wide programs, each taking the L2 cache region lock. Only three call sites read that association and all need just an identifier: compulsory data elements (E1303, E1076), payload membership (E1305) and program rule effect membership. Drop the mapping and project the needed data elements with one query per preheat instead, bounded by the compulsory count plus payload and rule width rather than by program width.
The rule engine runs after validation and DataValuesValidator runs again afterwards, on the payload an ASSIGN effect mutated. Such an effect can add a data value for a data element the original payload did not carry, so the projected data elements of the stage have to cover it or that second pass rejects valid data with E1305. They do, because TrackerIdentifierCollector preheats the data elements of all program rule actions, which is what ProgramStageDataElementsSupplier probes with. Nothing pinned that invariant, so narrowing the collection to the payload's programs would have broken the projection silently.
Query the two id lists as arrays with = any() instead of in (). An empty in () list is not valid SQL, which needed a sentinel uid that could never match. An array is well formed when empty and still uses dataelement_uid_key, so the sentinel is gone. It also keeps the statement text independent of the payload width, where in (...) expands to one placeholder per element. Name the stage collections after program stages rather than stages, drop the benchmark numbers and what the supplier already explains from the surrounding javadoc, and split the compulsory data element codes: E1303 reports one missing from the event, E1076 one the event deletes.
The supplier claimed the payload and program rule data elements are both preheated. That only holds under idScheme=UID: TrackerIdentifierCollector adds the rule ones as uids but into the same identifier set as the payload's, which is queried in the payload's idScheme, so under CODE, NAME or ATTRIBUTE they never load and silently drop out of the probe set. Also say that memberUids holds the same data elements as members rather than implying it is a separate selection.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24773 +/- ##
============================================
- Coverage 58.05% 55.05% -3.01%
+ Complexity 1414 327 -1087
============================================
Files 3725 3729 +4
Lines 144739 144958 +219
Branches 16875 16897 +22
============================================
- Hits 84033 79806 -4227
- Misses 53550 57611 +4061
- Partials 7156 7541 +385
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1868 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The issue
Uganda run a custom capture app that batches up entries and syncs them through
/api/tracker. On 2.42.5.1, at ~200 RPS on tracker import, requests stall: a single import trace shows 82s of wall time for 1.3s of CPU, with ~89% of samples parked on a lock.What makes their instance unusual is the metadata, not the payload. Their trace shows 1281 data element loads in a single import, against 3 to 50 data elements per stage in the Sierra Leone demo programs. A payload referencing a handful of data values pays for all of them.
That is because preheat maps
Programthrough MapStruct, which walksProgramStage.programStageDataElementsand dereferences each element'sdataElement. Nothing batches this, Hibernate'shibernate.default_batch_fetch_sizeis unset repo-wide so it defaults to off, so it is one entity load per data element. The queries themselves are fast, 1.039s for all 1281, so the DB is not the problem.The problem is what each of those loads touches.
DataElement.hbm.xmldeclares<cache usage="read-write"/>, and in Hibernate 5.6 that strategy is served byAbstractReadWriteAccess, the only class incache/spi/supportholding aReentrantReadWriteLock. That lock is per region, not per key, there is no striping, so everyDataElementkey serializes through one lock. Reads share it, but every cache miss takes it exclusively and blocks readers of every other data element. It is also the non-fair constructor, so a reader arriving while a writer is merely queued parks behind it rather than barging.Multiply that by the N+1 (1281 acquisitions per request) and by concurrent imports and the region never returns to the cheap shared state. Threads spend their time parked rather than working, which is why the trace shows 82s of wall against 1.3s of CPU.
Where the time goes
Wall samples under
TrackerImportController, fromprofile.collapsedof the profiled run below.ProgramStageDataElement.hashCode(triggers the proxy init)AbstractReadWriteAccess(L2 region lock)Unsafe.park(threads parked on that lock)loadFromDatasourceProgramStageDataElementsSupplier(the new query)These overlap rather than sum, they are nested frames. The lock is gone and the replacement query does not take its place.
Fix
Only three call sites read
programStageDataElementsduring import, and all three need an identifier and nothing else:DataValuesValidatorE1303, E1076DataValuesValidatorE1305RuleActionEventMapperThe last two are membership tests whose probes are known up front, so a data element that neither the payload nor the rules mention cannot change the outcome and does not need to be fetched.
So drop
@Mapping(target = "programStageDataElements")and add a dedicated preheat supplier,ProgramStageDataElementsSupplier, which fetches all of it in one JDBC query instead of one Hibernate entity load per data element. That removes the N+1 and, because it does not go through Hibernate, it never touches the L2 cache or its region lock:Bounded by
|compulsory| + |payload + rules|rather than by the width of the stage. The compulsory branch still scales with the program, so the gain shrinks the more of a stage is compulsory. The widened program measured below has none, and Uganda's compulsory count is not known.Unioned rather than
ored: anorspanning both tables cannot use an index, so Postgres reads every row of the stage and ofdataelementand filters after. Split, the second branch usesdataelement_uid_key.Data elements referenced by the payload or by program rules are still loaded as full entities by
DataElementStrategy, since validation needs theirvalueTypeandoptionSet. That path is untouched, and it is a single batchedINquery rather than the per-element loads removed here. What goes away is loading the stage's other data elements, the ones no payload or rule mentions.Sidenote: tracker's own preheat cache has been hardcoded off for years (
DefaultPreheatCacheService.isCacheEnabled()returnsfalse, config read commented out), sotracker.import.preheat.cache.enabledis dead config. Worth revisiting separately: caching a few things explicitly, with a deliberate TTL and without Hibernate's per-entity serialization, looks more promising than relying on L2 for this.Performance
TrackerTestwas adapted to attach ~2000 extra data elements to the MNCH / PNC program stages before the run, to get within range of Uganda's 1281 loads per import. Stock Sierra Leone programs are far too narrow to reproduce this. See TrackerTest#widenMnchProgram on d725da3.32 concurrent import users, 180s, Sierra Leone DB. Zero failed requests on both sides of both runs.
-e wall, the source of the frame table aboveMedian Response Time (p50) (ms)
95th Percentile Response Time (p95) (ms)
Throughput 2.61 -> 6.06 req/s, 492 -> 1118 imports in the same window.
The gain scales with how many data elements the program stages carry. On a stock Sierra Leone program there is nothing to remove; this only pays off on wide programs like Uganda's.
Still to do