Skip to content

perf(mapper): pre-resolve mapper classes#1852

Merged
brendt merged 1 commit into3.xfrom
mapping-performance
Jan 7, 2026
Merged

perf(mapper): pre-resolve mapper classes#1852
brendt merged 1 commit into3.xfrom
mapping-performance

Conversation

@brendt
Copy link
Member

@brendt brendt commented Jan 6, 2026

Closes #1834

@brendt brendt changed the base branch from main to 3.x January 6, 2026 08:55
@brendt brendt requested a review from innocenzi January 6, 2026 09:09

foreach ($this->config->mappers as $mapperClass) {
/** @var Mapper $mapper */
$mappers[] = $this->container->get($mapperClass, context: $context);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this cache make it so that a mapper could potentially receive a dependency with a stale state?

Copy link
Member

Choose a reason for hiding this comment

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

Can you give me a scenario for this?

Copy link
Member

Choose a reason for hiding this comment

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

Imagine a value object registered as a singleton at the start of the request. The mappers are resolved and cached. The value object is changed in the container for any reason, the next time the mapper that uses it is called, it will have the previous value?

Might be niche though, but could be a pain to debug

Copy link
Member

Choose a reason for hiding this comment

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

Sounds pretty nice indeed, but possible. Perhaps we could just add a refresh() method that'd essentially do:

public function refresh(): self
{
    $this->mappers = $this->resolveMappers();

    return $this;
}

The above is a global refresh. We could also play with the idea of giving fresh mappers to the caller only:

public function refresh(): self
{
    $clone = clone($this);
    $clone->mappers = $clone->resolveMappers();

    return $clone;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I completely understand the problem?

Wouldn't this cache make it so that a mapper could potentially receive a dependency with a stale state?

Mappers themselves aren't singletons (they can't be because they require that context variable), so whenever they are retrieved from the container, it's a fresh new object.

Copy link
Member

Choose a reason for hiding this comment

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

It's more about dependencies than mappers being singletons or not.

Let's say you have this:

class SomeMapper implements Mapper {
    public function __construct(private SomeSingletonService $service) {}
}
  1. Request start, ObjectFactory is created, mappers are resolved and cached
  2. SomeSingletonService gets re-registered for whatever reason mid-request
  3. SomeMapper gets called, but it still holds the old SomeSingletonService instance

It's very niche and antipattern, but a possibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha. Yeah I don't think that we need to consider that edge case because, as you say, that's a clear antipattern. I can only imagine this happening during internal framework testing, and in that case there would be better solutions.

@brendt brendt merged commit 49fc4e2 into 3.x Jan 7, 2026
1 check passed
@brendt brendt deleted the mapping-performance branch January 7, 2026 08:15
@brendt
Copy link
Member Author

brendt commented Jan 7, 2026

Gonna merge, we can further reiterate on it if needed.

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.

[Performance] ORM and DI performance issues due to lack of cache

3 participants