GH-73: Derive a class's mapping shape once, not per element - #119
Merged
Conversation
Everything a class's declaration fixes - property list, replace map,
collector, constructor, required flags, defaults - was re-derived through
fresh reflection on every mapSingleObject() call. For a collection that is
once per ELEMENT. Measured with a counting extractor:
before 1 element -> 1 derivation ... 100 elements -> 100
after any element count -> 1 per class
and 100 elements of one class went from 8.9 ms to 3.2 ms.
ClassMetadata holds the shape, ClassMetadataFactory derives it, and the
memo is per INSTANCE. That replaces the process-wide "static $cache" in
unknownPropertyCollector() - the library's only global state, sitting in a
final readonly class, contradicting the project's own rule and outliving
any test that touched it.
Not routed through the PSR-6 pool the issue suggests: the metadata holds a
ReflectionMethod, which does not survive serialisation. Deriving it is
cheap once it happens once per class, and an in-memory memo is what the
measured cost called for.
294 lines left JsonMapper, which is what the issue estimated.
Two things the tooling caught rather than me. PHPStan found
getReflectionClass() left dead by the move. And jscpd found that keeping
hasAttribute() and constructorParameter() in both classes was 42 lines of
duplication - the right answer was not to share the helpers but to move
what they were used FOR: whether a property replaces null with its
default, and what its default is, are declaration facts, so they belong to
the metadata like everything else here. Both JsonMapper methods became
one-line delegations.
… the static The review found three defects, all mine, two of them silent. A declared default may be an EXPRESSION, and reflection evaluates it when asked for the value. Caching the resolved value per class therefore got two things wrong. An unused default ran anyway - a promoted `new X()` whose property the payload supplies now constructed X, and a throw from it escaped as a native error past the report. And the evaluated object was frozen into the metadata, so every element of a collection received the SAME instance: mutate it on one, mutate it on all. Neither was covered by the 377 tests. The metadata now caches the reflection HANDLE carrying the default and evaluates it per call. The per-element cost the issue targets - the lookup - stays memoised; the evaluation returns to per-use, as it was before this branch. Two regression tests pin it: an unused throwing default, and a mutable default distinct per element. And the `static $cache` the commit message claimed to have removed was still there - moved verbatim into unknownPropertyCollector(), static included. So the library's only global state had survived the change whose headline was removing it. Gone now, the per-instance memo already giving the once-per-class behaviour it duplicated. While fixing the eager evaluation I also collected the constructor's promoted parameters once per class rather than re-reflecting the class for every property, which the review measured at ~1.8x on a wide class with a narrow payload.
hasRequiredConstructorArguments() compared itself to constructorForHydration()
via {@see}, but that method moved to ClassMetadataFactory in this branch and
is private there, so the reference resolved to nothing. Stated as prose
naming the class, which is what a cross-class private detail warrants.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
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.
Fixes #73.
Per
mapSingleObject()call — that is, per COLLECTION ELEMENT — the mapper re-derived through fresh reflection everything a class's declaration already fixes: the property list, theReplacePropertymap, the constructor analysis, and per property the required check and default. Only the property type was cached. Measured with a counting extractor:and 100 elements of one class dropped from 8.9 ms to 3.2 ms.
The shape
ClassMetadata(a readonly value object) holds what the declaration fixes;ClassMetadataFactoryderives and memoises it per instance. That memo replaces thestatic $cacheinunknownPropertyCollector()— the library's only global state, in afinal readonlyclass, contradicting the project's own rule. 294 lines leftJsonMapper.Not routed through the PSR-6 pool the issue suggests: the metadata holds a
ReflectionMethod, which does not survive serialisation. Deriving it is cheap once it happens once per class, and an in-memory memo is what the measured cost called for.What review caught — three defects, all mine, two silent
An unused default was evaluated. A declared default can be an expression, and reflection evaluates it when asked for the value. Caching the resolved value per class meant a promoted
new X()ran even when the payload supplied the property — and a throw from it escaped as a native error past the report.An object default was shared across elements. The evaluated object, frozen into the metadata, was handed to every element of a collection: mutate it on one, mutate it on all. Silent shared state between objects that never met. Neither defect was covered by the then-377 tests.
Both fixed by caching the reflection handle and evaluating per call — the per-element cost the issue targets (the lookup) stays memoised; the evaluation returns to per-use. Two regression tests pin it.
The
static $cacheI claimed to have removed was still there — moved verbatim, static included, so the global state the change's headline was removing had survived it. Gone now.Also, while fixing the eager evaluation, the constructor's promoted parameters are collected once per class rather than re-reflected per property — the review measured ~1.8× on a wide class with a narrow payload otherwise.
Two things the tooling caught rather than me: PHPStan found
getReflectionClass()left dead by the move, and jscpd found 42 lines of helper duplication — resolved by moving what the helpers were used for (replace-null flag, default value) into the metadata, since those are declaration facts too.Acceptance criteria
staticcache; metadata resolved through an injectable, cacheable component (grep -rn 'static $' src/is empty).composer ci:testgreen, verified by exit code: 379 tests, 1229 assertions; PHPStan max, Rector, CGL and CPD clean.