Index indexed children rather than scanning per element - #51556
Open
tommyk-gears wants to merge 2 commits into
Open
Index indexed children rather than scanning per element#51556tommyk-gears wants to merge 2 commits into
tommyk-gears wants to merge 2 commits into
Conversation
Binding an indexed element scanned every property name in the source to discover which indices exist, using filter(root::isAncestorOf). With many indexed elements in a single namespace, for example a large Map whose values contain lists, this made binding quadratic in the number of elements. Add a getChildrenOf method that returns the names directly beneath a given name. The default implementation performs the same scan as before, so sources that cannot answer more cheaply are unaffected. SpringIterableConfigurationPropertySource overrides it and answers from mappings built during the pass its cache already makes over property names, turning each lookup into a map access. The children mappings are kept separate from the existing descendants set for now. They have the same key set, so containsDescendantOf could later be answered from them, but that is left alone here to keep this change small. Binding a Map of 717 entries whose values each contain two lists drops from 133ms to 30ms (best of 25 runs, measured either side of this commit). Signed-off-by: Tommy Karlsson <tommy.karlsson@leovegas.com>
The cache built a descendants set purely so that containsDescendantOf could answer without scanning. Now that it also records each name against its parent, that set is redundant: the children mappings have the same key set, so containsDescendantOf can ask whether a name has any children. Removes the descendants set, addParents and the capture flag. The children mappings are always built, since getChildrenOf needs them for every source, and the ancestorOfCheck guard is kept in containsDescendantOf, where a mapper with a custom check still needs its own ancestor logic. Like addParents before it, addChildren stops walking as soon as it reaches a parent that is already known. Whichever name first recorded that parent walked the rest of the way up, so everything above it is already present. Without that the mappings would take roughly twice as long to build. Building the mappings for 6000 property names takes 3.6-4.2ms, against 3.6-4.1ms for the descendants set it replaces. Signed-off-by: Tommy Karlsson <tommy.karlsson@leovegas.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.
IndexedElementsBinderdiscovers which indices exist beneath a name by scanning everyproperty name in the source:
The indexed property-name syntax carries no length, so the only evidence that
tags[0]exists is that some name happens to look like
tags[0].name. That means one full scan perindexed element. When many indexed elements share a namespace — a large
Mapwhose valuescontain lists, say — binding becomes quadratic in the number of elements.
Binding a
Mapwhose values each contain two lists:The per-element cost stops growing with the size of the source.
Changes
Add
getChildrenOftoIterableConfigurationPropertySource. The default implementationperforms the same scan as before, so sources that cannot answer more cheaply are unaffected.
SpringIterableConfigurationPropertySourceoverrides it and answers from mappings builtduring the pass its cache already makes over property names.
Answer
containsDescendantOffrom those mappings. The cache built a separate descendantsset purely so
containsDescendantOfcould avoid scanning. The children mappings have thesame key set, so that set is now redundant and is removed along with
addParentsand itscapture flag.
Like
addParentsbefore it,addChildrenstops walking as soon as it reaches a parent thatis already known — whichever name first recorded that parent walked the rest of the way up.
Without that short-circuit the mappings take roughly twice as long to build. With it,
building the mappings for 6000 property names takes 3.6-4.2ms, against 3.6-4.1ms for the
descendants set they replace.
The
ancestorOfCheckguard is kept incontainsDescendantOf, since a mapper with a customcheck (
SystemEnvironmentPropertyMapper) still needs its own ancestor logic. The childrenmappings themselves are built for every source, because
getChildrenOfneeds themregardless of which mapper is in use.
Notes
getChildrenOfhas one caller today. I looked at the otherisAncestorOf/isParentOfsites (
NoUnboundElementsBindHandler,ValidationBindHandler,assertNoUnboundChildren); they all want descendants rather than direct children, sonone migrate cleanly.
Setper parent wherethe descendants set held one flat set of parents.
written differently (
foo-bar/fooBar), and system environment sources.