Skip to content

Improve ReflectionEnum->getCases() performance #1410

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: 6.30.x
Choose a base branch
from
+51 −9
Open

Improve ReflectionEnum->getCases() performance #1410

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
72a45e4
Improve ReflectionEnum->getCases() performance
staabm Mar 22, 2024
7aa8054
Remove unnecessary array_values()
staabm Mar 22, 2024
a7ff65c
memoize cases in ReflectionEnum
staabm Mar 22, 2024
7948bc2
pull out repeated isBacked() of the loop
staabm Mar 22, 2024
12a5789
Implement Memoize<T>
staabm Mar 22, 2024
cf29bb0
Discard function after use
staabm Mar 22, 2024
646e4e1
incorporate feedback
staabm Mar 22, 2024
3de5a55
missing return
staabm Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions src/Reflection/Adapter/ReflectionEnum.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Roave\BetterReflection\Reflection\ReflectionMethod as BetterReflectionMethod;
use Roave\BetterReflection\Reflection\ReflectionProperty as BetterReflectionProperty;
use Roave\BetterReflection\Util\FileHelper;
use Roave\BetterReflection\Util\Memoize;
use ValueError;

use function array_combine;
Expand All @@ -33,9 +34,27 @@
*/
final class ReflectionEnum extends CoreReflectionEnum
{
/** @var Memoize<list<ReflectionEnumUnitCase>|list<ReflectionEnumBackedCase>> */
private Memoize $cases;

public function __construct(private BetterReflectionEnum $betterReflectionEnum)
{
unset($this->name);

$this->cases = new Memoize(function() {
$isBacked = $this->betterReflectionEnum->isBacked();
$cases = $this->betterReflectionEnum->getCases();

$mappedCases = [];
foreach ($cases as $case) {
if ($isBacked) {
$mappedCases[] = new ReflectionEnumBackedCase($case);
} else {
$mappedCases[] = new ReflectionEnumUnitCase($case);
}
}
return $mappedCases;
});
}

/** @return non-empty-string */
Expand Down Expand Up @@ -527,17 +546,10 @@ public function getCase(string $name): ReflectionEnumUnitCase|ReflectionEnumBack
return new ReflectionEnumUnitCase($case);
}

/** @return list<ReflectionEnumUnitCase|ReflectionEnumBackedCase> */
/** @return list<ReflectionEnumUnitCase>|list<ReflectionEnumBackedCase> */
public function getCases(): array
{
/** @psalm-suppress ImpureFunctionCall */
return array_map(function (BetterReflectionEnumCase $case): ReflectionEnumUnitCase|ReflectionEnumBackedCase {
if ($this->betterReflectionEnum->isBacked()) {
return new ReflectionEnumBackedCase($case);
}

return new ReflectionEnumUnitCase($case);
}, array_values($this->betterReflectionEnum->getCases()));
return $this->cases->memoize();
}

public function isBacked(): bool
Expand Down
36 changes: 36 additions & 0 deletions src/Util/Memoize.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

declare(strict_types=1);

namespace Roave\BetterReflection\Util;

/** @template T */
Copy link
Member

@Ocramius Ocramius Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of this, meanwhile:

/** 
 * @template T
 * @internal do not touch: you have been warned.
 */
final class Memoize
{
    private readonly mixed $cached;
    /** @param pure-Closure(): T $cb */
    public function __construct(private Closure|null $cb) {}
    public function get(): mixed {
        if ($this->cb) {
            $this->cached = ($this->cb)();
            $this->cb = null;
       }
        
        return $this->cached;
    }
}

This could be used general-purpose on most adapters, if needed.

Copy link
Member

@Ocramius Ocramius Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly what you wrote, except I don't care if $cached is null (valid result)

Copy link
Contributor Author

@staabm staabm Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - copied it over. the SA tools don't like it ;)

Copy link
Member

@Ocramius Ocramius Mar 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's good :D

We are indeed bending the rules here: hence the @internal stuff as well.

I'll likely need to take over on this: a good measure of error suppression will be needed, but the impure-closure bit is most annoying (it shouldn't consider it impure 🤔 )

final class Memoize {
/**
* @var T
* @psalm-suppress PossiblyNullPropertyAssignmentValue
*/
private mixed $cached = null;

/**
* @var (callable(): T)|null
*/
private $fn;

/** @param callable(): T $fn */
public function __construct(callable $fn)
{
$this->fn = $fn;
}

/**
* @return T
*/
public function memoize(): mixed {
if ($this->cached === null && $this->fn !== null) {
$this->cached = ($this->fn)();
$this->fn = null;
}
return $this->cached;
}
}