[Serializer] Improve ObjectNormalizer performance #16547
Conversation
| } elseif (strpos($name, 'is') === 0) { | ||
| // issers | ||
| $attributes[lcfirst(substr($name, 2))] = true; | ||
| } |
There was a problem hiding this comment.
Aren't the constructor checks redundant here? They would never get through the has/is/get checks, or is it done simply because of performance reasons?
Personally I would like to see a guard clause here for readability:
if ($reflMethod->getNumberOfRequiredParameters() > 0) {
continue;
}
// also prevents a lot of is checks as destructor is hardly ever implemented and constructor only ever onceAnother option would be to omit the number of required attributes at this point and only check it when needed.
$attributes[lcfirst(substr($name, 3))] = 0 === $reflMethod->getNumberOfRequiredParameters();This would limit the checks even more, but I'm not sure if calling the reflection method is slower or relying on getName() and strpos()
If it comes down to preferences, feel free to ignore if you disagree
|
Can you rebase? Thanks. |
da10798 to
6f02eb1
Compare
|
Rebased. I've also refactored the code to add a guard clause according to @iltar comment but I left the constructor/destructor for now as we don't know the impact on performance such check can have. |
| return self::$attributesCache[$key]; | ||
| } | ||
|
|
||
| if (self::$attributesCache[$key] = $this->getAllowedAttributes($object, $context, true)) { |
There was a problem hiding this comment.
you should not assign false here. It could corrupt the cache in case an exception is thrown in the following logic, is catched, and the method is called again: as you assign false in the cache array, the cache is set and used on future calls (and breaks the contracts of the method). Use a local variable and assign the cache only on next line when using the value.
and the logic is also altered here: an empty array should be returned here. Only false triggers the fallback logic. So you need to use a strict comparison
There was a problem hiding this comment.
btw, this means we are missing a test covering the case where the object does not have any property serialized in the groups being used (your PR switches it to serialize the whole object, which is a regression)
e2eba98 to
ab6c947
Compare
|
@stof bug fixed and test added. Thanks for the review. |
ab6c947 to
861a34e
Compare
|
Thank you @dunglas. |
…ct class (dunglas) This PR was squashed before being merged into the 3.1-dev branch (closes #17191). Discussion ---------- [Serializer] Move the normalization logic in an abstract class | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a As suggested by @xabbuh, move all the normalization logic for objects in an abstract class. It will ease the maintenance as well as adding new features such as #17113 and #16143. I've introduced a new abstract class to avoid BC breaks in `AbstractNormalizer`. As a (good) side effect, all normalizers now benefits from the caching system introduced in #16547. Commits ------- 3bec813 [Serializer] Move the normalization logic in an abstract class
Cache attributes detection in a similar way than in #16294 for PropertyAccess.
As for the PropertyAccess Component, I'll open another PR (in 2.8 or 3.1) allowing to cache these attributes between requests using Doctrine Cache.
@Tobion, can you try this PR with your benchmark to estimate the gain?