Skip to content

Commit

Permalink
Optimize TypeRegistry::lookupName() from O(N) to O(1) (doctrine#6082)
Browse files Browse the repository at this point in the history
|      Q       |   A
|------------- | -----------
| Type         | performance issue
| Fixed issues | n/a

#### Summary

We use a lot of custom types and migration from `Type::getName()` to
`TypeRegistry->lookupName()` discovered us the reverse lookup was very
slow (O(N)). This PR makes it O(1).

~Index is initialized lazily on the 1st reverse lookup.~ The type
registry is expected to not hold duplicate type instances and this PR
fixes it, in
doctrine#6083 (comment) I was
requested to fix it in this PR instead of a separate one.
  • Loading branch information
mvorisek authored and cgknx committed Aug 30, 2023
1 parent 63fffec commit a670b51
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 16 deletions.
36 changes: 20 additions & 16 deletions src/Types/TypeRegistry.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@

use Doctrine\DBAL\Exception;

use function array_search;
use function in_array;
use function spl_object_id;

/**
* The type registry is responsible for holding a map of all known DBAL types.
Expand All @@ -17,11 +16,17 @@ final class TypeRegistry
{
/** @var array<string, Type> Map of type names and their corresponding flyweight objects. */
private array $instances;
/** @var array<int, string> */
private array $instancesReverseIndex;

/** @param array<string, Type> $instances */
public function __construct(array $instances = [])
{
$this->instances = $instances;
$this->instances = [];
$this->instancesReverseIndex = [];
foreach ($instances as $name => $type) {
$this->register($name, $type);
}
}

/**
Expand All @@ -31,11 +36,12 @@ public function __construct(array $instances = [])
*/
public function get(string $name): Type
{
if (! isset($this->instances[$name])) {
$type = $this->instances[$name] ?? null;
if ($type === null) {
throw Exception::unknownColumnType($name);
}

return $this->instances[$name];
return $type;
}

/**
Expand Down Expand Up @@ -77,7 +83,8 @@ public function register(string $name, Type $type): void
throw Exception::typeAlreadyRegistered($type);
}

$this->instances[$name] = $type;
$this->instances[$name] = $type;
$this->instancesReverseIndex[spl_object_id($type)] = $name;
}

/**
Expand All @@ -87,15 +94,18 @@ public function register(string $name, Type $type): void
*/
public function override(string $name, Type $type): void
{
if (! isset($this->instances[$name])) {
$origType = $this->instances[$name] ?? null;
if ($origType === null) {
throw Exception::typeNotFound($name);
}

if (! in_array($this->findTypeName($type), [$name, null], true)) {
if (($this->findTypeName($type) ?? $name) !== $name) {
throw Exception::typeAlreadyRegistered($type);
}

$this->instances[$name] = $type;
unset($this->instancesReverseIndex[spl_object_id($origType)]);
$this->instances[$name] = $type;
$this->instancesReverseIndex[spl_object_id($type)] = $name;
}

/**
Expand All @@ -112,12 +122,6 @@ public function getMap(): array

private function findTypeName(Type $type): ?string
{
$name = array_search($type, $this->instances, true);

if ($name === false) {
return null;
}

return $name;
return $this->instancesReverseIndex[spl_object_id($type)] ?? null;
}
}
8 changes: 8 additions & 0 deletions tests/Types/TypeRegistryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,14 @@ public function testRegisterWithAlreadyRegisteredInstance(): void
$this->registry->register('other', $newType);
}

public function testConstructorWithDuplicateInstance(): void
{
$newType = new TextType();

$this->expectException(Exception::class);
new TypeRegistry(['a' => $newType, 'b' => $newType]);
}

public function testOverride(): void
{
$baseType = new TextType();
Expand Down

0 comments on commit a670b51

Please sign in to comment.