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

Performance penalty getClassMetadata #2355

Closed
sirian opened this issue Aug 5, 2021 · 4 comments
Closed

Performance penalty getClassMetadata #2355

sirian opened this issue Aug 5, 2021 · 4 comments
Labels
Stale Issues that have not seen any activity in 30 days

Comments

@sirian
Copy link

sirian commented Aug 5, 2021

Problem

Each call to $documentManager->getClassMetadata($class) causes ClassMetadataFactory to resolve real class again and again. This causes huge performance penalty when documentManager actively used.

$metadata = $this->metadataFactory->getMetadataFor($className);

return parent::getMetadataFor($this->dm->getClassNameResolver()->getRealClass($className));

Tests

I made some tests. 10000 calls to getClassMetadata consumes ~14.7ms

$start = microtime(true);
for ($i = 0; $i < 10000; $i++) {
    $dm->getClassMetadata(User::class);
}
var_dump(microtime(true) - $start);

Tests with cache

Now I extend DocumentManger and add caching, and run tests again. 10000 iterations consumes 0.76ms (20x times faster!)

class DocManager extends DocumentManager
{
    private array $classMetadataCache = [];

    public function getClassMetadata($className): ClassMetadata
    {
        return $this->classMetadataCache[$className] ??= parent::getClassMetadata($className);
    }
}
@franmomu
Copy link
Contributor

franmomu commented Aug 5, 2021

Thanks for reporting! can you please try it with the 2.3.x branch?

Class name resolvers were removed in #2304 (apparently there was cache in the class name resolving)

$this->classNameResolver = new CachingClassNameResolver(new ProxyManagerClassNameResolver($this->config));

@alcaeus
Copy link
Member

alcaeus commented Aug 6, 2021

To follow up on this, we do have a few benchmarks for performance critical stuff. @sirian if you feel up for it, you can add a performance test in the benchmark in a PR to 2.2.x. The test should repeatedly fetch metadata from the document manager for a non-proxy class and a proxy class (to test class name resolution). We'd expect mean performance to be very similar after a number of iterations, since only the first call is expected to be slower.

As mentioned by @franmomu, this was removed in 2.3.x now that doctrine/persistence can take care of this. When building the new proxy system, I realised that we'd have to make extensive changes to doctrine/persistence to account for the different proxy naming scheme, which I postponed due to the workload involved. I'd expect this to me significantly more efficient in 2.3.x, but in case we do spot performance issues there, we might want to fix them in doctrine/persistence before releasing 2.3.x. Since ODM is the only manager currently proxies other than those from doctrine/common, there's no real performance testing going on of the logic in doctrine/persistence.

Please let us know if you're interested in making the changes or if you need help with it.

@stale
Copy link

stale bot commented Sep 6, 2021

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Issues that have not seen any activity in 30 days label Sep 6, 2021
@stale stale bot closed this as completed Apr 18, 2022
@malarzm
Copy link
Member

malarzm commented Apr 18, 2022

I do believe this is fine now with caching in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues that have not seen any activity in 30 days
Projects
None yet
Development

No branches or pull requests

4 participants