fix: switch reference-metadata L2 cache regions to NONSTRICT_READ_WRITE (2.41) - #24827
Draft
jason-p-pickering wants to merge 2 commits into
Draft
fix: switch reference-metadata L2 cache regions to NONSTRICT_READ_WRITE (2.41)#24827jason-p-pickering wants to merge 2 commits into
jason-p-pickering wants to merge 2 commits into
Conversation
…TE (2.41) 2.41 port of the cache-strategy change in #24810. Hibernate 5.6's generic cache SPI (org.hibernate.cache.spi.support.AbstractReadWriteAccess) backs READ_WRITE region locking for both Ehcache3/JCache (master) and the legacy Ehcache2 hibernate-ehcache module 2.41 still runs (confirmed: StorageAccessImpl implements the same DomainDataStorageAccess SPI) - the per-region, non-striped ReentrantReadWriteLock that #24810 fixes on master applies here too, not just under JCache. Flips the same reference-metadata regions #24810 flips - Category*, CategoryOption*, DataElement*, Indicator*, Legend*, Option*, OrganisationUnit*, PeriodType - from read-write to nonstrict-read-write in 2.41's .hbm.xml mappings (2.41 has no Java @Cache annotations; all L2 mapping is hbm.xml-based). Period/RelativePeriods intentionally excluded, matching #24810. Three entity-level regions - CategoryCombo, LegendSet, OptionSet - exist only on 2.41 (master doesn't cache those entities at all, only their collections), so #24810 never touches them; left as read-write here to match its exact scope rather than expanding it unreviewed. Out of scope, deliberately: #24810's ehcache.xml predefined-region/ store-by-reference changes address JCache's jsr107 MissingCacheStrategy default (store-by-value via SerializingCopier) - 2.41's Ehcache2 caches are already store-by-reference by default, so that part doesn't apply. The HibernateJobConfigurationStore synchronized-query-space fix is a separate concern (#24803's wipe fix) not requested here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Master's #24810 predefines hot Hibernate L2 regions as ehcache-native caches to escape hibernate-jcache's forced store-by-value on JSR-107-created caches. That specific problem doesn't apply here: Ehcache 2's native API is always store-by-reference regardless of whether a region is predefined or created on demand via CacheManager#addCacheIfAbsent, so 2.41 never paid the SerializingCopier cost #24810 fixes. What does carry over: without an explicit entry, each of these regions falls back to defaultCache and shares its single 1,000,000-entry cap with everything else. Porting #24810's measured hot-region list and heap bounds into Ehcache 2's native XML syntax gives the busiest regions their own sized, bounded cache instead of competing for headroom in the shared default. org.hisp.dhis.option.OptionSet.options is deliberately excluded: that collection is intentionally left uncached on 2.41 (see OptionSet.hbm.xml) to avoid an N+1 where a cached id list combined with an entity-cache miss makes Hibernate resolve ids one at a time. Predefining the region here would be dead configuration at best. Verified by loading the file through net.sf.ehcache's own ConfigurationFactory/CacheManager: all 29 regions parse and register. 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
1 participant
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.
2.41 port of the cache-strategy portion of #24810 (Morten Svanæs), scoped down per Morten's head-to-head comment on #24815 confirming the region-lock mechanism (and NONSTRICT_READ_WRITE as the fix) holds up under real concurrent load on master.
Why this applies to 2.41 too
2.41 runs the legacy
hibernate-ehcachemodule (Ehcache2,net.sf.ehcache2.10.9.2) rather than master's Ehcache3/JCache stack, so it was worth checking whether Morten's finding — Hibernate'sREAD_WRITEaccess strategy takes oneReentrantReadWriteLockper region, not per key, so every cache miss blocks readers of every other key in that region — is specific to the JCache integration before porting blind.It isn't. In Hibernate 5.6,
hibernate-ehcache'sStorageAccessImplimplements the sameorg.hibernate.cache.spi.support.DomainDataStorageAccessSPI that the JCache region factory does (verified by decompilinghibernate-ehcache-5.6.15.Final.jar) — the actualREAD_WRITE/NONSTRICT_READ_WRITElocking logic, includingAbstractReadWriteAccess's per-region lock, lives in Hibernate's genericcache.spi.supportpackage and is shared by both region factories. Ehcache2 vs Ehcache3 changes only the storage backend underneath; the lock granularity problem #24810 fixes is unchanged.What's ported vs. deliberately left out
Ported: flips the same reference-metadata regions #24810 flips —
Category*,CategoryOption*,DataElement*,Indicator*,Legend*,Option*,OrganisationUnit*,PeriodType— fromread-writetononstrict-read-write. 2.41 has no Java@Cacheannotations (all L2 mapping is still.hbm.xml-based), so this is a.hbm.xmlattribute change rather than aCacheConcurrencyStrategyenum change.Period/RelativePeriodsintentionally excluded, matching #24810.Three entity-row regions —
CategoryCombo,LegendSet, andOptionSetitself (i.e. the entity's own row: id/name/etc., cached at the<class>level) — are cached on 2.41 but have no equivalent@Cacheannotation on master'sCategoryCombo/LegendSet/OptionSetclasses at all; master only caches (a subset of) their collections. Since #24810 never touches a region that doesn't exist, these three entity-row regions are left asread-writehere rather than expanding scope beyond what's actually been reviewed/tested upstream.To be explicit about the one that matters most for the Option N+1 investigation this stack is part of:
OptionSet.options— the collection region, not the entity-row region above — is flipped tononstrict-read-writehere, matching #24810'sOptionSet.javachange exactly. Only the unrelatedOptionSetentity-row cache (line-level metadata likename/valueType, nothing to do with the options list) is the one left untouched.Deliberately not ported:
ehcache.xmlpredefined-region / store-by-reference block. That addresses JCache's jsr107MissingCacheStrategy.CREATEdefaulting on-demand caches to store-by-value (SerializingCopierrunning inside the lock). 2.41's Ehcache2 caches are store-by-reference by default already (2.41 ran Ehcache2 by-reference for years) — nothing to fix here.HibernateJobConfigurationStoresynchronized-query-space change. That's fix: declare synchronized entity classes on native writes DHIS2-21963 #24803's wholesale-cache-wipe fix bundled into perf: fix the L2 cache region-lock convoy on concurrent metadata-heavy load #24810 for standalone testability; a 2.41 port of fix: declare synchronized entity classes on native writes DHIS2-21963 #24803 (if needed) should happen as its own PR, not smuggled in here.Testing
.hbm.xmlchanges validated withxmllint --noout;dhis-service-corecompiles clean. This is a config-only change (no Java touched), so no new unit tests. No 2.41-specific load test has been run yet — the case for this PR is the mechanism argument above plus 2.41's own earlier canary (#24755, draft/do-not-merge) which validatedNONSTRICT_READ_WRITEcombined with two other changes and found no correctness regression, though that run didn't isolate this change's effect under concurrency on its own. Opening as a draft pending that isolation, or reviewer sign-off that the mechanism argument is sufficient.Related: #24810, #24815, #24773, #24803.
🤖 AI Assisted