Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Refactored AnnotationDriver to handle only required classes #256

Closed
wants to merge 1 commit into from

4 participants

@mattcockayne

Whilst debugging an issue I was having I found that the AnnotationDriver uses get_declared_classes() and iterates over the result to identify classes that should be handled.

This seems like a ridiculous overhead especially as it then instantiated a ReflectionClass for each declared class. As an example my current project was iterating over 522 items when it needed to only handle 6.

I've kept the alterations to a minimum but it now takes a snapshot of the declared classes before and after the require for the files and then uses array_diff to get only the classes we are targeting.

@mattcockayne mattcockayne Refactored to handle only required classes
Refactored getAllClasses method to man that it only parses and
handles the files/classes required for the driver. This saves
unnecessary iteration and instantiation of Reflection objects
that are not needed and just add overhead
b6ae3e0
@doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DCOM-176

@stof
Collaborator

This is wrong. What if some of the mapped classes were already loaded before calling this method ?

@Ocramius
Owner

@mattcockayne this is wrong: if classes were loaded before directories are scanned, you got broken metadata.

@mattcockayne

Ok... Im willing to accept Im wrong... however surely there has to be a better way

@Ocramius
Owner

@mattcockayne are you sure you want to have performance optimizations here? This part of the logic is never called with cached metadata.

@mattcockayne

To be honest I wouldn't have even been digging around here if I hadn't been trying to debug a different issue.

I appreciate that its not called when the metadata is cached but it feels wrong for it to be unnecessarily iterating over unrelated files and creating so many reflection classes.

Surely any optimisation is a good thing (as long as it doesnt break anything, which I am now being told I have done)

@Ocramius
Owner

@mattcockayne the code you modified affects performance by an un-perceptible amount. Any file under the configured directory IS relevant to the annotation driver

@stof
Collaborator

Thus, any code using getAllClasses should be questioned for places being performance-sensitive. The only places where this method is used currently AFAIK is for utility tasks (generating entities for instance). The runtime code is always getting the metadata for a given class.

@mattcockayne

@Ocramius I accept that its only a very minor change and it is imperceptible in terms of performance.

I only really had need to alter it due to the fact that in trying to debug something using xdebug I ended up hitting a loop of 500+ items (not fun to have to step through) when I only needed to work with the 6 that were in the directory that I had configured the driver to work with.

I apologise for failing to understand how classes that form part of PHP core and have no relationship to the entities I'm working with should be included in the iteration I have altered.

In the end I suppose you could say that my change is mainly aesthetic in value but not totally invalid in its intention.

@mattcockayne

@stof The only reason I even noticed this is because I'm currently trying to debug an unrelated issue I'm having with a project which makes use of the doctrine cli tools

@Ocramius
Owner

@mattcockayne simply move your entities to a namespace where there's entities only and you got the problem solved :) (or use a different mapping driver)

@mattcockayne

@Ocramius I'm sorry but Im still not understanding.. .

My entities are already in a separate directory(i.e. 'src/MyModule/Entities'), with separate dedicated namespaces (i.e. 'MyModule\Entities') and with separate drivers defined for each relevant module.

However when the getAllClasses method is run it calls $declared = get_declared_classes();

This then returns an array of ALL classes defined. At the point of execution for my project this includes classes that are part of PHP core and classes from my project that are irrelevant to the entities I'm working with.

It then iterates over this whole array and instantiates a ReflectionClass for each and uses that reflection class to then test it its file ($rc->getFileName()) exists in an array of files retrieved from a previous scan of the directory that I have configured my dedicated driver to work with.

My concern is that it firstly iterates over so many items that are unrelated and secondly that it creates a ReflectionClass for each one. Surely this is unnecessary?

I appreciate that my changes are flawed but I cant say that I am happy with the current implementation.

@Ocramius
Owner

@mattcockayne that's how it works right now, yes: can't do anything about it.

You could work with the assumption that people use PSR-0, but that's not the case, since that's a huge assumption.

The only assumption we can do is the file name of the required file and the declaring file, and that's the only way to get it as far as I know.

@stof
Collaborator

@mattcockayne If you find another way to know exactly what are all the classes defined in a specific folder, let us know. But your proposal does not work to achieve it. It will only find classes that were not yet loaded, which is wrong (simply try to call the getAllClasses method twice and the second call will return an empty array)

@mattcockayne

@Ocramius you are correct in that making that assumption is wrong and I realised my own mistake shortly after creating the pull request.

@stof As I've said I appreciate that my solution is flawed. I had hoped that someone else might have been able to offer some insight into a way to solev this. To be honest I am not sure of there is a way to achieve a solution without resorting to some form of regex.

In the mean time I've closed my pull request till I can find a more appropriate solution

@Ocramius
Owner

@mattcockayne it would be interesting if there was some API that tells us the declared classes per file instead :)

@mattcockayne

@Ocramius funnily enough Im currently trawling through the PHP source code to see if there is some form of undocumented feature in php, however I'm struggling to spot anything.

There does appear to be an internal reference that stores the information I am wanting but it will take someone with better skills than I have to implement something that will leverage it.

closest I can find from a PHP perspective is http://wiki.birth-online.de/snippets/php/get-classes-in-file perhaps with an array_walk/array_map rather than a foreach it may be a possible solution

@mattcockayne

@Ocramius obviously it would need to be adjusted to take into account T_NAMESPACE of course

@Ocramius
Owner

@mattcockayne the code you linked is really too slow for this use case... You could eventually mail internals and ask there...

@stof
Collaborator

@mattcockayne token_get_all is a bad idea as soon as the file become big. It will probably drop the performances instead of increasing them.

@mattcockayne

@stof I had suspected that it wouldn't have been a good solution. unfortunately I cant see anyone offering any alternative.

such a shame :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 1, 2013
  1. @mattcockayne

    Refactored to handle only required classes

    mattcockayne authored
    Refactored getAllClasses method to man that it only parses and
    handles the files/classes required for the driver. This saves
    unnecessary iteration and instantiation of Reflection objects
    that are not needed and just add overhead
This page is out of date. Refresh to see the latest.
View
6 lib/Doctrine/Common/Persistence/Mapping/Driver/AnnotationDriver.php
@@ -173,6 +173,7 @@ public function getAllClassNames()
$classes = array();
$includedFiles = array();
+ $beforeDeclaration = get_declared_classes();
foreach ($this->paths as $path) {
if ( ! is_dir($path)) {
@@ -197,12 +198,13 @@ public function getAllClassNames()
}
}
- $declared = get_declared_classes();
+ $afterDeclaration = get_declared_classes();
+ $declared = array_diff($afterDeclaration, $beforeDeclaration);
foreach ($declared as $className) {
$rc = new \ReflectionClass($className);
$sourceFile = $rc->getFileName();
- if (in_array($sourceFile, $includedFiles) && ! $this->isTransient($className)) {
+ if (in_array($sourceFile, $includedFiles) && !$this->isTransient($className)) {
$classes[] = $className;
}
}
Something went wrong with that request. Please try again.