Much faster method for getting Metadata of a class #152

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@michaelperrin

Since commit cc6673 resolving issue #2688, generating entities with the app/console doctrine:generate:entities command have become very slow, especially with many entities.

The problem comes from the getMetadataForClass method of the Doctrine\Bundle\DoctrineBundle\Mapping\MetadataFactory class which was looking for the right class after getting the metadata of all classes.
I changed the implementation so that only the metadata of the given class is retrieved.
Generating my entities is now about 12 times faster in my project.

Maybe we should consider updating all these classes to the latest version of the jms metadata project (https://github.com/schmittjoh/metadata) in the future.

edit: removed last sentence

Much faster method for getting Metadata of a class
Instead of looking for the right class after getting the metadata of all classes, just get the metadata of the given class.
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Jan 16, 2013

Member

These classes have nothing to do with the JMS Matadata library

Member

stof commented Jan 16, 2013

These classes have nothing to do with the JMS Matadata library

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 16, 2013

Indeed, sorry!

Indeed, sorry!

Mapping/MetadataFactory.php
- }
+ foreach ($this->registry->getManagers() as $em) {
+ $class = $this->getClassMetadataFactoryClass();
+ $cmf = new $class();

This comment has been minimized.

@stloyd

stloyd Jan 16, 2013

Contributor

Both lines could be moved outside loop.

@stloyd

stloyd Jan 16, 2013

Contributor

Both lines could be moved outside loop.

Mapping/MetadataFactory.php
+ $cmf->setEntityManager($em);
+
+ $metadata = $cmf->getMetadataFor($entity);
+ return new ClassMetadataCollection(array($metadata));

This comment has been minimized.

@stloyd

stloyd Jan 16, 2013

Contributor

Missing new line before return.

@stloyd

stloyd Jan 16, 2013

Contributor

Missing new line before return.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 16, 2013

Thanks @stloyd for your comments. I just made a new commit dealing with your comments.
I actually tried to mimic the code style of other methods of the same class, but that wasn't a good idea!

Thanks @stloyd for your comments. I just made a new commit dealing with your comments.
I actually tried to mimic the code style of other methods of the same class, but that wasn't a good idea!

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Jan 29, 2013

@stloyd Did you have time to check the latest coding style changes I made? Tell me if anything else needs to be done.

@stloyd Did you have time to check the latest coding style changes I made? Tell me if anything else needs to be done.

Mapping/MetadataFactory.php
+
+ $metadata = $cmf->getMetadataFor($entity);
+
+ return new ClassMetadataCollection(array($metadata));

This comment has been minimized.

@stof

stof Mar 5, 2013

Member

This looks wrong to me. It always return for the first entity manager even when it is not the manager related to the entity.

@stof

stof Mar 5, 2013

Member

This looks wrong to me. It always return for the first entity manager even when it is not the manager related to the entity.

This comment has been minimized.

@michaelperrin

michaelperrin Mar 6, 2013

Fixed

Retrieve class metadata for all entity managers
An entity could have several entity managers associated to it
(which doesn't happen often I guess). Metadata from the right entity
manager is now retrieved anyway.
@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Mar 6, 2013

@stof problem regarding entity managers is now fixed.

@stof problem regarding entity managers is now fixed.

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Aug 30, 2013

Any news on this PR? Any comments are welcome.

Any news on this PR? Any comments are welcome.

@Padam87

This comment has been minimized.

Show comment
Hide comment
@Padam87

Padam87 Sep 20, 2013

IMO this would be better:

    private function getMetadataForClass($entity)
    {
        $em = $this->registry->getManagerForClass($entity);

        if ($em === null) {
            return new ClassMetadataCollection(array());
        }

        $class = $this->getClassMetadataFactoryClass();
        $cmf = new $class();
        $cmf->setEntityManager($em);

        $metadata = $cmf->getMetadataFor($entity);

        return new ClassMetadataCollection(array($metadata));
    }

A microtime diff test on a small app(~15 entities, 1 manager), for a single entity:
current(in lib): float(0.25397801399231)
after: float(0.003140926361084)

@michaelperrin 👍 , this would improve speed a lot for a large app

Padam87 commented Sep 20, 2013

IMO this would be better:

    private function getMetadataForClass($entity)
    {
        $em = $this->registry->getManagerForClass($entity);

        if ($em === null) {
            return new ClassMetadataCollection(array());
        }

        $class = $this->getClassMetadataFactoryClass();
        $cmf = new $class();
        $cmf->setEntityManager($em);

        $metadata = $cmf->getMetadataFor($entity);

        return new ClassMetadataCollection(array($metadata));
    }

A microtime diff test on a small app(~15 entities, 1 manager), for a single entity:
current(in lib): float(0.25397801399231)
after: float(0.003140926361084)

@michaelperrin 👍 , this would improve speed a lot for a large app

stof added a commit that referenced this pull request Oct 12, 2013

Improved the way to load metadata for a single class
There is no need to load metadata for all classes in this case.
Replaces #152
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 12, 2013

Member

I fixed it a bit differently because your code misses the check about classes being transient before calling getMetadataFor, and because it was conflicting with my recent change

Member

stof commented Oct 12, 2013

I fixed it a bit differently because your code misses the check about classes being transient before calling getMetadataFor, and because it was conflicting with my recent change

@stof stof closed this Oct 12, 2013

@michaelperrin

This comment has been minimized.

Show comment
Hide comment
@michaelperrin

michaelperrin Oct 12, 2013

@stof Great!

@stof Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment