perf: cache program rule variable option-set mappings (2.41) - #24852
Draft
jason-p-pickering wants to merge 4 commits into
Draft
perf: cache program rule variable option-set mappings (2.41)#24852jason-p-pickering wants to merge 4 commits into
jason-p-pickering wants to merge 4 commits into
Conversation
DefaultProgramRuleEntityMapperService.getOptions(ProgramRuleVariable) called
optionSet.getOptions() fresh on every tracker import that touched the owning
program, for every option-set-backed rule variable with useCodeForOptionSet
false. OptionSet.options has no L2 collection cache (deliberately, to avoid
a different N+1), so every call re-queried optionvalue and re-populated the
Option entity cache without ever reading from it - visible on a live
instance as ehcache_puts_total{cache="Option"} climbing steadily against a
flat ehcache_hits_total, correlated with ProgramRuleAction cache hits, while
the option set itself hadn't changed in over a week.
Memoize the mapped (name, code) list per option set id via a new
CacheProvider-backed cache, matching this module's existing
programRuleVariablesCache pattern: TTL-bounded (1 hour) rather than
event-invalidated on Option/OptionSet writes, since option sets change far
less often than that in practice.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r dependency The prior commit added a CacheProvider constructor param and called createProgramRuleVariableOptionsCache() eagerly in the constructor, but left the test's @Injectmocks wiring untouched. Since no @mock CacheProvider existed, Mockito's constructor injection passed null, so the constructor threw an NPE for every test in the class (previously masked by running against a stale .m2 build during initial verification). Switch to explicit construction with a real SimpleCacheBuilder-backed cache instead of a mock, since stubbing a mock's method after @Injectmocks already ran is too late - the field value is already captured. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The mapping cache added previously was TTL-only (1h), which meant an edited/added/removed Option could take up to an hour to show up in rule-variable evaluation. Option/OptionSet writes arrive via DefaultOptionService, the generic metadata CRUD controller, or a metadata import - none of which funnel through a single call site DefaultProgramRuleEntityMapperService could hook into directly, so invalidation has to happen at the Hibernate level instead. Add OptionCacheInvalidationListener, a PostCommit insert/update/delete listener for the Option entity, registered globally via OptionCacheInvalidationListenerConfigurer - the same registration pattern DeletedObjectListenerConfigurer already uses. This is same-node only; a write committed on another node in a cluster still relies on the existing 1h TTL as a backstop. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Hibernate's PostInsert/PostUpdate/PostDeleteEventListener interfaces all extend Serializable, making this class transitively Serializable even though it's just a Spring singleton registered with Hibernate's EventListenerRegistry and never actually serialized. SonarQube flagged the non-transient, non-serializable field (S1948); transient satisfies that contract without changing runtime behaviour. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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
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.



Problem
DefaultProgramRuleEntityMapperService.getOptions(ProgramRuleVariable)calledoptionSet.getOptions()fresh on every tracker import that touched the owning program, for every option-set-backed rule variable withuseCodeForOptionSetfalse.OptionSet.optionshas no L2 collection cache (deliberately, to avoid a different N+1), so every call re-queriedoptionvalueand re-populated theOptionentity cache without ever reading from it — visible on a live instance asehcache_puts_total{cache="Option"}climbing steadily against a flatehcache_hits_total, correlated withProgramRuleActioncache hits, while the option set itself hadn't changed in over a week.Also directly visible in query-stats profiling as a
SELECT ... FROM optionset WHERE optionsetid = ?fired once per rule-variable-mapping pass per option set, because any non-id method call on a Hibernate proxy (like.getOptions()) forces the proxy to fully initialize itself first.Fix
Memoize the mapped
(name, code)list per option set id via a newCacheProvider-backed cache. On a cache hit, the code now only callsoptionSet.getId()— which Hibernate special-cases to never trigger proxy initialization — so theoptionsetrow load is skipped entirely instead of just its options collection.Cache invalidation
Optionwrites can arrive viaDefaultOptionService, the generic metadata CRUD controller, or a metadata import — none of which funnel through a single call site this class could hook into directly to invalidate the cache itself. So invalidation happens at the Hibernate level instead:OptionCacheInvalidationListeneris aPostCommitinsert/update/delete listener for theOptionentity, registered globally viaOptionCacheInvalidationListenerConfigurer— the same registration patternDeletedObjectListenerConfigureralready uses elsewhere in the codebase. On any Option write, it evicts just that option's owning option-set entry.This is same-node only. A write committed on another node in a cluster isn't seen by this listener, so cross-node staleness still relies on the pre-existing 1h TTL as a backstop.
What this does not fix
Worth being explicit about scope: this only caches the
OptionSet→Optionleg. It does not touch the separateDataElementproxy load that also shows up in the same query-stats profile (SELECT ... FROM dataelement WHERE dataelementid = ?) —toRuleVariable()/toMappedValueType()callprv.getDataElement().getUid()/.getValueType()/.hasOptionSet()unconditionally on every mapping pass, uncached, regardless of whether an option set is involved. That's a separate N+1 (no cache wrapstoMappedProgramRuleVariables()as a whole) and isn't addressed here.Testing
ProgramRuleEntityMapperServiceTestupdated for the newCacheProviderconstructor dependency (was relying on@InjectMockspassingnullfor anything unmocked, which broke once the constructor started callingcacheProvider.createProgramRuleVariableOptionsCache()eagerly — fixed with explicit construction stubbed with a realSimpleCacheBuilder-backed cache rather than a mock, since a mock stubbed after@InjectMocksalready ran is too late).OptionCacheInvalidationListenerTestcovers the listener's type/null-guard logic in isolation; mutation-tested (broke theinstanceof Optioncheck, confirmed all 5 tests fail correctly, restored).dhis-service-program-rulemodule: 41 tests, 0 failures.🤖 AI Assisted