Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Add first level cache in ValueObjectNormalizer #252

Conversation

@nlx-sascha
Copy link
Contributor

nlx-sascha commented Jan 1, 2020

This drastically increase performance for large set of events
that uses valueObjects in the constructor.

@nlx-sascha

This comment has been minimized.

Copy link
Contributor Author

nlx-sascha commented Jan 1, 2020

I use 100.000 Events with ~30 ValueObject in the constructor.

ReflectionNamedType::getName -11 961 816 calls
Neos\EventSourcing\EventStore\Normalizer\ValueObjectNormalizer_Original::resolveNamedConstructorMethod -6 080 757
Neos\Utility\TypeHandling::normalizeType -5 980 967 calls

Before ~300 Events / s
After ~2000 Events / s

@nlx-sascha nlx-sascha force-pushed the nlx-sascha:feature/first-level-cache-in-value-object-normalizer branch from 825c441 to d69c575 Jan 1, 2020
Copy link
Member

bwaidelich left a comment

Thanks a lot for this nice contribution! Just a couple of nitpicky comments

@@ -21,6 +21,9 @@
*/
final class ValueObjectNormalizer implements DenormalizerInterface
{
protected $resolveConstructorMethodCache = [];

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 2, 2020

Member
Suggested change
protected $resolveConstructorMethodCache = [];
/**
* @var array
*/
private $resolveConstructorMethodCache = [];

This comment has been minimized.

Copy link
@nlx-sascha

nlx-sascha Jan 2, 2020

Author Contributor

Done

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 2, 2020

Member

nitpick: It's still protected for no reason :)

@@ -21,6 +21,9 @@
*/
final class ValueObjectNormalizer implements DenormalizerInterface
{
protected $resolveConstructorMethodCache = [];
protected $resolveNamedConstructorMethodCache = [];

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 2, 2020

Member
Suggested change
protected $resolveNamedConstructorMethodCache = [];
/**
* @var array
*/
private $resolveNamedConstructorMethodCache = [];

This comment has been minimized.

Copy link
@nlx-sascha

nlx-sascha Jan 2, 2020

Author Contributor

Done

This comment has been minimized.

Copy link
@nlx-sascha

nlx-sascha Jan 2, 2020

Author Contributor

Sorry didn't notice that you want the property private - finally done :)

@@ -44,6 +47,11 @@ public function supportsDenormalization($data, $className, $format = null): bool

private function resolveConstructorMethod(string $dataType, string $className): ReflectionMethod
{
$cacheIdentifier = md5($dataType . $className);

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 2, 2020

Member

To prevent the (frankly very unlikely) case of overlaps we should add a separator between the cache id parts:

Suggested change
$cacheIdentifier = md5($dataType . $className);
$cacheIdentifier = md5($dataType . '|' . $className);
return $constructorMethod;
}

private function resolveNamedConstructorMethod(string $dataType, string $className, \ReflectionClass $reflectionClass): ?ReflectionMethod
{
$cacheIdentifier = md5($dataType . $className);

This comment has been minimized.

Copy link
@bwaidelich

bwaidelich Jan 2, 2020

Member
Suggested change
$cacheIdentifier = md5($dataType . $className);
$cacheIdentifier = md5($dataType . '|' . $className);
@nlx-sascha nlx-sascha force-pushed the nlx-sascha:feature/first-level-cache-in-value-object-normalizer branch from d69c575 to ff3f2d6 Jan 2, 2020
This drastically increase performance for large set of events
that uses valueObjects in the constructor.
@nlx-sascha nlx-sascha force-pushed the nlx-sascha:feature/first-level-cache-in-value-object-normalizer branch from ff3f2d6 to 6e83079 Jan 2, 2020
Copy link
Member

bwaidelich left a comment

Awesome, thanks!

@bwaidelich bwaidelich merged commit 049af2e into neos:master Jan 2, 2020
1 check passed
1 check passed
continuous-integration/styleci/pr The analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.