Memoize system environment resolution in NavigableMapConfig - #16130
Draft
codeconsole wants to merge 1 commit into
Draft
Memoize system environment resolution in NavigableMapConfig#16130codeconsole wants to merge 1 commit into
codeconsole wants to merge 1 commit into
Conversation
Every Config.getProperty call resolved the key against the process environment from scratch. findInSystemEnvironment asked resolvePropertyName for the environment spelling of the key, and checkPropertyName probed System.getenv up to four times per casing - as-is, dots replaced, hyphens replaced, both replaced - then repeated the whole sequence against the uppercased key. That is up to eight getenv probes and six intermediate strings per lookup, and the answer never changes: the process environment is fixed for the lifetime of a config. Resolution is now memoized per config instance, as is the token list a dotted key splits into. Property values themselves are not cached, so configuration that changes at runtime is still observed. The cache is deliberately per-instance rather than static. Tests install environment variables reflectively and then build a fresh config, which must see the environment as it stands at that point. Measured on the grails-core config specs: getProperty drops from 355.3 to 91.6 ns/op over two million lookups. On a scaffolded page rendering 100 rows, NavigableMapConfig falls from 7.01% to 1.16% of JFR execution samples, the fields plugin being the caller that resolves configuration per rendered property.
codeconsole
marked this pull request as draft
August 11, 2026 00:12
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16130 +/- ##
==================================================
+ Coverage 52.3676% 52.4622% +0.0947%
- Complexity 18313 18322 +9
==================================================
Files 2036 2032 -4
Lines 96365 96315 -50
Branches 16832 16840 +8
==================================================
+ Hits 50464 50529 +65
+ Misses 38480 38354 -126
- Partials 7421 7432 +11
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 5ed8094 Learn more about TestLens at testlens.app. |
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.
What
Config.getPropertyresolves every key against the process environment from scratch on each call.findInSystemEnvironmentasksresolvePropertyNamefor the environment spelling of the key, andcheckPropertyNameprobesSystem.getenvup to four times per casing — as-is, dots replaced with underscores, hyphens replaced with underscores, both replaced — then repeats the whole sequence against the uppercased key:That is up to eight
getenvprobes and six intermediate strings per lookup, and the answer cannot change — the process environment is fixed for the lifetime of a config.This memoizes the resolution per config instance, along with the token list a dotted key splits into:
Property values are not cached, so configuration mutated at runtime is still observed:
The cache is per-instance rather than static on purpose.
SystemEnvironmentConfigSpecinstalls environment variables reflectively and then builds a fresh config, which must observe the environment as it stands at that point; a static cache would leak a stale answer across those specs.Why it matters
Callers resolve configuration inside render loops. On a scaffolded page rendering 100 rows,
FormFieldsTemplateService(getShouldCache,findTemplate,getTemplateFor) and asset-pipeline resolve configuration per rendered property, so this sits on a hot path that scales with rows × properties.Measurements
getProperty, 2M lookups over a 4-key setNavigableMapConfigshare of JFR execution samples, 100-row scaffolded page under loadLimitations
ExpandoMetaClassread-lock contention (~9–10% of samples) and reflective tag dispatch, neither of which this touches.On the
@DeprecatedmarkerNavigableMapConfigcarries a class-level@Deprecatedsince #11554 (May 2020), pointing atgrails.config.Config. That target is the interface this class implements, and no alternative implementation exists in the tree, so the note reads as "code against the interface" rather than "a replacement is available".The class is on the live path today:
PropertySourcesConfig extends NavigableMapConfigand is not itself deprecated.AbstractGrailsApplicationbuilds aPropertySourcesConfig, so this isgrailsApplication.configfor every running application.NavigableMapConfigat 7.01% of execution samples, entered throughgetProperty.If the
NavigableMap-backed implementation is replaced, this change goes with it. It is offered as a cost reduction on the path applications actually execute for the six years the marker has been in place, and is easy to drop if a replacement is in flight.