feat: introduce compiled normalizer cache #500

romm · 2024-03-17T22:40:47Z

This commit introduces a new compilation step for the normalizer, which aims to bring huge performance gains when normalizing any value. It works by adding a static analysis step to the process, which will recursively analyse how the normalizer should perform for every value it can meet. This process results in a native PHP code entry, that can then be cached for further usage.

The PHP code is an anonymous class that implements the Transformer interface, and is responsible of recursively transforming an input to the same structure, but with only arrays and scalar values. It is done by adding methods to the class, where basically each method is used to transform a specific type.

Example #1: a method that transforms a string, with a registered custom transformer.

(new MapperBuilder())
    ->registerTransformer(fn (string $value) => strtoupper($value))
    ->normalizer(Format::array())
    ->normalize('some string');

// Generated method for a string value:
private function transform_string_xyz(mixed $value): mixed
{
    $next = fn () => $value;
    if (\is_string($value)) {
        $next = fn () => $this->transformers[0]($value, $next);
    }
    return $next();
}

Example #2: a method that transforms a property of a class, which is typed as a shaped array.

final readonly class SomeClass
{
    public function __construct(
        /** @var array{foo: string, bar: int} */
        public array $someArray,
    ) {}
}

// Generated method for the shaped array property:
private function transform_shaped_array_xyz(array $value): array
{
    $result = [];
    foreach ($value as $key => $item) {
        $result[$key] = match ($key) {
            'foo' => $this->transform_string_xyz($item),
            'bar' => $this->transform_int_xyz($item),
            default => $this->transform_mixed($item),
        };
    }
    return $result;
}

The entry point to this process can be found in CacheTransformer where the following steps occur:

The value to be normalized is analyzed: an instance of Type is created, which will be the root type for the whole static analysis process.
The TransformerDefinitionBuilder will recursively fetch all types that can be met during the normalization. For instance, if the root type is a class, then all its properties and their types will be included in the process. For each type, transformers (registered with MapperBuilder::registerTransformer) are analyzed: if a transformer matches a type, it will be part of the compilation process.
For each type, a TypeFormatter will be called. These type formatters are responsible for manipulating the compiler which is used to create the PHP code that will normalize the value. How it is done really depends on the type and can be really simple (e.g. DateTimeFormatter) or quite complex (e.g. ClassFormatter).
The PHP code is then written into a file and included when needed.

…piled-normalizer # Conflicts: # src/Cache/KeySanitizerCache.php # src/Library/Settings.php # src/Normalizer/ArrayNormalizer.php # src/Normalizer/Formatter/JsonFormatter.php # src/Normalizer/Transformer/RecursiveTransformer.php # src/Normalizer/Transformer/ValueTransformersHandler.php # tests/Integration/Normalizer/NormalizerTest.php

…piled-normalizer # Conflicts: # src/Normalizer/Formatter/JsonFormatter.php

# Conflicts: # src/Library/Container.php # src/Normalizer/Transformer/KeyTransformersHandler.php # src/Normalizer/Transformer/ValueTransformersHandler.php

…piled-normalizer # Conflicts: # src/Normalizer/Formatter/JsonFormatter.php

Baptouuuu

I'm not versed enough into the project to every details of the transformers so I skimmed a bit over it.

Baptouuuu · 2025-03-08T07:46:23Z

src/Type/Types/ClassStringType.php

@@ -93,7 +123,7 @@ public function matches(Type $other): bool
        }

        if (! $other->subType) {
-            return false;
+            return true;


Was this a bug ?

It was indeed. I planned to do it in another commit but forgot. Thanks for noticing! 😊

Done in #615

Baptouuuu · 2025-03-08T07:50:35Z

src/Type/Types/IterableType.php

@@ -59,6 +60,8 @@ public function accepts(mixed $value): bool
        return true;
    }

+    public function compiledAccept(CompliantNode $node): CompliantNode {}


Does this need to be implemented ?

Yes! It's one of the few things left to be done.

Baptouuuu · 2025-03-08T07:56:05Z

src/Type/Types/ListType.php

+        return ! Polyfill::array_any(
+            $value,
+            fn (mixed $item, mixed $key) => ! $this->subType->accepts($item),
+        );


Instead of the double negative why not use array_all ?

Using array_all would mean looping over all of the values, whereas array_any will stop on the first invalid value. A small performance improvement, but less code readability maybe.

array_all stops iterating over the array if the callback returns false, the performance should be the same.

Baptouuuu · 2025-03-08T07:58:25Z

src/Type/Types/ListType.php

+                Node::functionCall(function_exists('array_any') ? 'array_any' : Polyfill::class . '::array_any', [
+                    $node,
+                    Node::shortClosure(
+                        Node::logicalOr(


Why the wrap in a logical or ? As I understand it so far, removing this will produce the same compiled code.

Good catch!

Baptouuuu · 2025-03-08T08:20:34Z

src/Compiler/Library/NewAttributeNode.php

+     * @param array<mixed> $arguments
+     * @return array<Node>
+     */
+    private function argumentNode(array $arguments): array


Since this method is recursive, it may be interesting to make it static to avoid capturing $this in the array_map closure for each recursion level.

Right. What about making the closure static? Do you think one of these solutions would be best?

Both the method and closure should be static.

If only the closure is, it will still need to capture the object via the $self = $this; use ($self) approach. But this is more complex and still has the same problem.

Baptouuuu · 2025-03-08T10:24:24Z

src/Normalizer/Transformer/Compiler/Definition/TransformerDefinition.php

+    public function markAsSure(): self
+    {
+        $self = clone $this;
+        $self->isSure = true;


Could you explain the meaning of this flag, I'm not sure a fully grasp its extent.

I added some documentation: d696f8b; is it enough to understand properly?

Perfect 👌

Baptouuuu · 2025-03-08T10:42:24Z

tests/Unit/Normalizer/Formatter/JsonStreamFormatterTest.php

@@ -12,7 +12,7 @@

 use const JSON_THROW_ON_ERROR;

-final class JsonFormatterTest extends TestCase
+final class JsonStreamFormatterTest extends TestCase


Why rename this test since the JsonFormatter hasn't changed its name ?

Good catch, it came from a change that was not commited (or rolled back).

Baptouuuu · 2025-03-08T10:53:32Z

tests/Integration/Normalizer/ExpectedCache/iterable-of-scalars-with-transformers.php

@@ -0,0 +1,103 @@
+<?php // Generated by CuyZ\Valinor\Cache\FileSystemCache
+return fn (array $transformers, CuyZ\Valinor\Normalizer\Transformer\Transformer $delegate) => new class ($transformers, $delegate) implements \CuyZ\Valinor\Normalizer\Transformer\Transformer {


Why fully qualify the interface in implements but not as the argument ?

🤷 probably made sense at some point, not anymore. I changed it, thanks.

Baptouuuu · 2025-03-08T10:59:04Z

tests/Integration/Normalizer/NormalizerCompiledCodeTest.php

+        // To help updating the expectation file, uncomment the following line
+        // and run the test once. Then, comment it back. Remember to check that
+        // the updated content is correct.
+        // file_put_contents($expectedFile, file_get_contents($cacheFiles[0]));


I'm not sure if it's relevant but it could be interesting to validate the cache file doesn't contain a syntax error.

Or are other tests on the compiled code enough ?

Other tests already make sure of that 😉

Baptouuuu · 2025-03-08T11:05:53Z

tests/Integration/Normalizer/NormalizerTest.php

+            ],
+        ];
+
+        yield 'positive integer with non-negative-int transformer' => [


It could be interesting to also test non-*-int transformers with the 0 value.

We need moooore tests! 😎

Thanks for the suggestion.

@Baptouuuu

From @Baptouuuu: > `Compliant` suggests the object is already in a compliant state, but `Compliance` express the its possibility.

…s compilation

romm added 10 commits April 10, 2024 14:48

feat: introduce compiled normalizer cache

ef69d13

wip

edd3009

wip

dd8ddfe

wip formatter

0a38375

wip CompliantNode

c625f0a

wip

f4a1643

wip

8f8b051

wip

4ce05cb

wip

9f0edda

CuyZ deleted a comment from Mediagone Oct 14, 2024

romm added 5 commits October 18, 2024 15:11

wip

45dddeb

wip

5022c14

wip

7209ce0

Merge remote-tracking branch 'refs/remotes/cuyz/master' into feat/com…

a6529c9

…piled-normalizer # Conflicts: # src/Normalizer/Formatter/JsonFormatter.php

wip

9b0306f

romm force-pushed the feat/compiled-normalizer branch from 064279c to 9b0306f Compare January 18, 2025 13:42

romm added 4 commits January 18, 2025 14:43

Merge remote-tracking branch 'cuyz/master' into feat/compiled-normalizer

e5591c6

# Conflicts: # src/Library/Container.php # src/Normalizer/Transformer/KeyTransformersHandler.php # src/Normalizer/Transformer/ValueTransformersHandler.php

wip

889f45a

wip

988ad37

Merge remote-tracking branch 'refs/remotes/cuyz/master' into feat/com…

27b47fc

…piled-normalizer # Conflicts: # src/Normalizer/Formatter/JsonFormatter.php

Baptouuuu reviewed Mar 8, 2025

View reviewed changes

romm added 8 commits March 9, 2025 19:07

misc: remove unnecessary compilation or

59306d4

qa: improve compiler node types

d09bd3c

fix: properly handle value node compilation for arrays

de73b2e

qa: improve compiler node types

f3caef0

misc: rename CompliantNode to ComplianceNode

e62bdfd

From @Baptouuuu: > `Compliant` suggests the object is already in a compliant state, but `Compliance` express the its possibility.

misc: update example docblock

568ea6b

misc: add doc to property

d696f8b

misc: rename test

bf51497

romm added 2 commits March 9, 2025 20:34

test: add normalizer tests for non-*-int type with zero value

6091bd2

fix: remove unnecessary leading backslash of interface implementation…

3877f1f

…s compilation

romm force-pushed the feat/compiled-normalizer branch from dd0a6c6 to 3877f1f Compare March 9, 2025 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce compiled normalizer cache #500

feat: introduce compiled normalizer cache #500

romm commented Mar 17, 2024 •

edited

Loading

Baptouuuu left a comment

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

romm Mar 9, 2025 •

edited

Loading

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 10, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 10, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 10, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

Baptouuuu Mar 8, 2025

romm Mar 9, 2025

		@@ -0,0 +1,103 @@
		<?php // Generated by CuyZ\Valinor\Cache\FileSystemCache
		return fn (array $transformers, CuyZ\Valinor\Normalizer\Transformer\Transformer $delegate) => new class ($transformers, $delegate) implements \CuyZ\Valinor\Normalizer\Transformer\Transformer {

feat: introduce compiled normalizer cache #500

Are you sure you want to change the base?

feat: introduce compiled normalizer cache #500

Conversation

romm commented Mar 17, 2024 • edited Loading

Baptouuuu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romm Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romm commented Mar 17, 2024 •

edited

Loading

romm Mar 9, 2025 •

edited

Loading