You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
PathTarget: drop the regex, cache subtype lists, clear context after a miss (plan items 3.8 + 2.5) - #4413
Plan item 3.8 (review D2), with correctness fix 2.5 folded in, as the plan says.
The problems
A regex per segment. Every segment of every filter, sort, update and projection path ran segment.matches("[0-9]+"), which compiles a regex each time.
A full subtype walk per miss. When a segment wasn't a property of the current model, resolveProperty rebuilt the full subtype set (a new HashSet) and recursed into each subtype, which rebuilt its own set again.
2.5, wrong translation after a miss. If no subtype had the segment either, context was left on the last subtype tried. With validation off, the next segment was then translated against a model it doesn't belong to. For example, unknown.radius became unknown.r because a subclass happened to map radius to r.
UpdateOperator.field() built a new PathTarget on every call, and the update codecs call it repeatedly.
Projection.knownFields ran a PathTarget and an extra getEntityModel per property, just to get back the property's own mapped name.
Change
Cached subtype list.EntityModel.allSubtypes() (@MorphiaInternal) returns every subtype below the model, nearest first, each once, and caches the list.
addSubtype invalidates it on the model and on every ancestor, through a new parent back-link. This includes interfaces, and subtypes mapped lazily after the first lookup. The walk visits each ancestor once, because a model is linked under every interface it inherits and following every path would be exponential.
The cached list is tagged with the version read before the walk, so a subtype linked during the walk makes it stale right away.
getSubtypes() now copies the cached list into a set.
PathTarget changes:
It checks for array operators by hand: $, $[…], or all digits.
It searches the model, then its cached subtype list, without recursion.
2.5: when nothing in the hierarchy has the segment, it sets context to null, so later segments pass through untranslated.
Its Class constructors use tryGetEntityModel(type) instead of isMappable followed by getEntityModel.
UpdateOperator.field() is not cached, though the plan asked for it. A subtype mapped later, for example while encoding another update's value, can change the translation, and nothing would invalidate the cache (found by Copilot). Resolution is cheap after this change. The plan item now records this.
Projection.knownFields puts each property's mapped name directly.
Tests
TestPathTargetSubtypes is offline and runs under both mappers:
Child and grandchild. A property on a subtype or a grandchild resolves from the root.
leavesLaterSegmentsAloneAfterAMiss (2.5).unknown.radius stays unknown.radius. It failed before the fix with unknown.r under both mappers.
resolvesThroughSubtypesMappedAfterTheFirstLookup. This is the plan's cache test: Circle, then the grandchild Ring, are mapped after earlier lookups on Shape, and their properties then resolve. The grandchild case checks that invalidation reaches ancestors.
resolvesThroughImplementationsMappedLater. The same, for an implementation linked under a mapped interface.
TestUpdateOperatorField checks that field() follows changes to its model, and to the mapping: a subtype mapped after the first call changes the translation.
prefersTheNearestSubtypeWhenAnIntermediateIsMappedLast: the nearer of two subtypes wins even when an intermediate class is mapped last.
linksADeepInterfaceChainRootFirstQuickly maps a root-first chain of 32 interfaces within a 5 s timeout. Recursive invalidation timed out.
PathTargetTest, TestUpdateOperations and TestQuery are unchanged and pass. The core and Kotlin suites pass with -Dmorphia.mapper=critter and with reflection: 1512 + 5 tests, 0 failures.
JMH: QueryBenchmark, master (3843251e286) vs this branch
CodecBenchmark doesn't exercise path resolution, so this uses QueryBenchmark from 0.2:
translatedPath resolves profile.address.city.
queryDocument builds a three-filter query document.
Same machine as before, -prof gc, master and this branch alternating, two rounds each.
variant
benchmark
master ns/op (2 runs)
this branch ns/op (2 runs)
master B/op
this branch B/op
reflection
translatedPath
1,021 ± 14, 1,010 ± 29
677 ± 21, 675 ± 22
5,228
2,276
reflection
queryDocument
3,198 ± 64, 3,187 ± 74
2,554 ± 59, 2,579 ± 155
14,028
9,160
critter
translatedPath
939 ± 21, 921 ± 11
601 ± 24, 595 ± 19
5,228
2,296
critter
queryDocument
3,210 ± 184, 3,087 ± 62
2,448 ± 53, 2,443 ± 92
14,560
9,304
Path translation is about 35% faster with 56% less allocation, and query documents are about 20% faster with 35% less allocation. The results are consistent across both rounds and both mappers.
Decisions to review
2.5 clears context rather than restoring it. The plan said "restore context". But once a segment matches nothing in the hierarchy, a later segment doesn't belong to the model we started from either. Restoring would still translate it against the wrong model, just a different one. Clearing means nothing after an unknown segment is translated, and with validation on, the unknown segment still fails as before.
Subtype search order is now deterministic: nearest first, by inheritance distance. It used to depend on HashSet iteration order. Distance means the fewest superclass or interface steps up to the model, not link order, because a subtype mapped before its superclass stays linked under a farther ancestor (found by Copilot). This only matters if two subtypes map the same Java property name to different stored names. Before, which one won was arbitrary; now the nearest subtype wins.
EntityModel gains a parent back-link (parents), used only to invalidate the cached subtype lists. It's populated by addSubtype, which is the only place hierarchy links are made since 2.2.
Not done here: the review's aside that knownFields omits the discriminator key, unlike project(). That's a behavior change outside 3.8, and I've left it for a separate decision.
…xt after a miss (plan items 3.8 + 2.5)
Resolving a path compiled a regex for every segment and, when a segment
wasn't on the current model, rebuilt and hashed the subtype set and
recursed into each subtype, rebuilding its set again. When no subtype had
the segment it left the context on the last subtype tried, so with
validation off a later segment was translated against a model it doesn't
belong to (2.5).
EntityModel now caches every subtype below it, nearest first, invalidated
through a parent back-link whenever a subtype is linked anywhere below it.
PathTarget checks for array operators by hand, searches that list without
recursing, and clears its context when nothing in the hierarchy has the
segment. UpdateOperator caches its translated field until its datastore,
model or validation setting changes, and Projection.knownFields uses the
mapped names directly.
A model is linked under every interface it inherits, so invalidating
subtype lists recursively through parents followed every path to each
ancestor, which is exponential in an interface chain's depth and runs
under the mapper's registration lock. It now visits each ancestor once.
UpdateOperator.field() no longer caches its translation: a subtype mapped
later, for instance while encoding another update's value, can change it,
and nothing would invalidate the cache.
For Deep extends Middle extends Root and Near extends Root, registering Root, Deep, Middle, Near leaves a shortcut from Root to Deep because AbstractMapper.linkHierarchy() does not remove it. This traversal then visits Deep before Near, so if both map radius differently, PathTarget chooses the farther subtype's stored name; registering Middle before Deep instead selects Near. Order candidates by actual inheritance distance, or normalize obsolete shortcut links, and add a regression with the intermediate class mapped last.
A subtype mapped before its superclass is linked under a farther ancestor,
and that link stays once the superclass is mapped, so link order isn't
nearest-first. allSubtypes() now orders by the fewest superclass or
interface steps up to the model's type, keeping link order for ties.
Re round 2's previously missed finding Order hierarchy candidates by inheritance distance: agreed, and it applies to a claim in my description. Fixed in 80d9e6d.
allSubtypes() now sorts the subtypes it finds by actual inheritance distance: the fewest superclass or interface steps from each subtype's type up to the model's type. The sort is stable, so ties keep link order. The order no longer depends on the shortcut links left behind when an intermediate class is mapped late.
The regression is TestPathTargetSubtypes.prefersTheNearestSubtypeWhenAnIntermediateIsMappedLast, under both mappers. Far extends Middle extends Base and Near extends Base both map radius, to far_r and near_r. With registration order Base, Far, Middle, Near, radius on Base now resolves to near_r. It failed with far_r before the fix.
Full core + Kotlin suites pass on both mappers (1512 + 5 tests).
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
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.
Plan item 3.8 (review D2), with correctness fix 2.5 folded in, as the plan says.
The problems
segment.matches("[0-9]+"), which compiles a regex each time.resolvePropertyrebuilt the full subtype set (a newHashSet) and recursed into each subtype, which rebuilt its own set again.contextwas left on the last subtype tried. With validation off, the next segment was then translated against a model it doesn't belong to. For example,unknown.radiusbecameunknown.rbecause a subclass happened to mapradiustor.UpdateOperator.field()built a newPathTargeton every call, and the update codecs call it repeatedly.Projection.knownFieldsran aPathTargetand an extragetEntityModelper property, just to get back the property's own mapped name.Change
EntityModel.allSubtypes()(@MorphiaInternal) returns every subtype below the model, nearest first, each once, and caches the list.addSubtypeinvalidates it on the model and on every ancestor, through a new parent back-link. This includes interfaces, and subtypes mapped lazily after the first lookup. The walk visits each ancestor once, because a model is linked under every interface it inherits and following every path would be exponential.getSubtypes()now copies the cached list into a set.PathTargetchanges:$,$[…], or all digits.contexttonull, so later segments pass through untranslated.Classconstructors usetryGetEntityModel(type)instead ofisMappablefollowed bygetEntityModel.UpdateOperator.field()is not cached, though the plan asked for it. A subtype mapped later, for example while encoding another update's value, can change the translation, and nothing would invalidate the cache (found by Copilot). Resolution is cheap after this change. The plan item now records this.Projection.knownFieldsputs each property's mapped name directly.Tests
TestPathTargetSubtypesis offline and runs under both mappers:leavesLaterSegmentsAloneAfterAMiss(2.5).unknown.radiusstaysunknown.radius. It failed before the fix withunknown.runder both mappers.resolvesThroughSubtypesMappedAfterTheFirstLookup. This is the plan's cache test:Circle, then the grandchildRing, are mapped after earlier lookups onShape, and their properties then resolve. The grandchild case checks that invalidation reaches ancestors.resolvesThroughImplementationsMappedLater. The same, for an implementation linked under a mapped interface.TestUpdateOperatorFieldchecks thatfield()follows changes to its model, and to the mapping: a subtype mapped after the first call changes the translation.prefersTheNearestSubtypeWhenAnIntermediateIsMappedLast: the nearer of two subtypes wins even when an intermediate class is mapped last.linksADeepInterfaceChainRootFirstQuicklymaps a root-first chain of 32 interfaces within a 5 s timeout. Recursive invalidation timed out.PathTargetTest,TestUpdateOperationsandTestQueryare unchanged and pass. The core and Kotlin suites pass with-Dmorphia.mapper=critterand with reflection: 1512 + 5 tests, 0 failures.JMH:
QueryBenchmark,master(3843251e286) vs this branchCodecBenchmarkdoesn't exercise path resolution, so this usesQueryBenchmarkfrom 0.2:translatedPathresolvesprofile.address.city.queryDocumentbuilds a three-filter query document.Same machine as before,
-prof gc, master and this branch alternating, two rounds each.Path translation is about 35% faster with 56% less allocation, and query documents are about 20% faster with 35% less allocation. The results are consistent across both rounds and both mappers.
Decisions to review
contextrather than restoring it. The plan said "restorecontext". But once a segment matches nothing in the hierarchy, a later segment doesn't belong to the model we started from either. Restoring would still translate it against the wrong model, just a different one. Clearing means nothing after an unknown segment is translated, and with validation on, the unknown segment still fails as before.HashSetiteration order. Distance means the fewest superclass or interface steps up to the model, not link order, because a subtype mapped before its superclass stays linked under a farther ancestor (found by Copilot). This only matters if two subtypes map the same Java property name to different stored names. Before, which one won was arbitrary; now the nearest subtype wins.EntityModelgains a parent back-link (parents), used only to invalidate the cached subtype lists. It's populated byaddSubtype, which is the only place hierarchy links are made since 2.2.knownFieldsomits the discriminator key, unlikeproject(). That's a behavior change outside 3.8, and I've left it for a separate decision.