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

fix: skip node build for NullType when value is not null #526

Merged
merged 3 commits into from
Apr 12, 2024

Conversation

NanoSector
Copy link
Contributor

Version 1.11 revamped union types but also deleted a fast path present in 1.10. In our use cases with many large objects with many nullable properties, this caused execution times to nearly double, especially with XDebug enabled.

The following test case sees its execution time drop from ~6.7 seconds to ~3.6 seconds on my local machine with the included patch. Profiling the test sees a reduction of node build calls to nearly half the original value.

<?php

declare(strict_types=1);

namespace CuyZ\Valinor\Tests\Integration\Mapping;

use CuyZ\Valinor\Mapper\MappingError;
use CuyZ\Valinor\Mapper\Source\Source;
use CuyZ\Valinor\Tests\Integration\IntegrationTestCase;

final class LotsOfObjectsMappingTest extends IntegrationTestCase
{
    public function test_can_map_lots_of_objects(): void
    {
        $objects = [];

        for ($i = 0; $i < 10000; ++$i) {
            $objects[] = [
                'valueA' => 'foo',
                'valueB' => 'bar',
                'valueC' => [
                    'valueA' => 'baz',
                    'valueB' => 'qux',
                ],
            ];
        }

        try {
            $result = $this->mapperBuilder()
                ->mapper()
                ->map(
                    'array<' . SomeClassWithNullableProperties::class . '>',
                    Source::array($objects)
                );
        } catch (MappingError $error) {
            $this->mappingFail($error);
        }

        self::assertCount(10000, $result);
    }
}

final class SomeClassWithNullableProperties
{
    public function __construct(
        public string                               $valueA,
        public string                               $valueB,
        public SomeClassWithNullableProperties|null $valueC = null,
        public SomeClassWithNullableProperties|null $valueD = null,
    ) {}
}

NanoSector and others added 3 commits April 10, 2024 20:01
Version 1.11 revamped union types but also deleted a fast path present in 1.10. In our use cases with many large objects with many nullable properties, this caused execution times to nearly double, especially with XDebug enabled.

The following test case sees its execution time drop from ~6.7 seconds to ~3.6 seconds on my local machine with the included patch. Profiling the test sees a reduction of node build calls to nearly half the original value.

```php
<?php

declare(strict_types=1);

namespace CuyZ\Valinor\Tests\Integration\Mapping;

use CuyZ\Valinor\Mapper\MappingError;
use CuyZ\Valinor\Mapper\Source\Source;
use CuyZ\Valinor\Tests\Integration\IntegrationTestCase;

final class LotsOfObjectsMappingTest extends IntegrationTestCase
{
    public function test_can_map_lots_of_objects(): void
    {
        $objects = [];

        for ($i = 0; $i < 10000; ++$i) {
            $objects[] = [
                'valueA' => 'foo',
                'valueB' => 'bar',
                'valueC' => [
                    'valueA' => 'baz',
                    'valueB' => 'qux',
                ],
            ];
        }

        try {
            $result = $this->mapperBuilder()
                ->mapper()
                ->map(
                    'array<' . SomeClassWithNullableProperties::class . '>',
                    Source::array($objects)
                );
        } catch (MappingError $error) {
            $this->mappingFail($error);
        }

        self::assertCount(10000, $result);
    }
}

final class SomeClassWithNullableProperties
{
    public function __construct(
        public string                               $valueA,
        public string                               $valueB,
        public SomeClassWithNullableProperties|null $valueC = null,
        public SomeClassWithNullableProperties|null $valueD = null,
    ) {}
}
```

Signed-off-by: NanoSector <rick@nanosector.nl>
@romm romm enabled auto-merge (squash) April 12, 2024 12:47
@romm
Copy link
Member

romm commented Apr 12, 2024

Very good, thank you @NanoSector! I pushed some minor changes.

@romm romm merged commit 6fad94a into CuyZ:master Apr 12, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants