alekitto / class-finder

Utility classes to help discover other classes/namespaces
https://alekitto.github.io/class-finder/
MIT License
27 stars 5 forks source link

Room for optimisation for ComposerFinder #21

Closed fogrye closed 2 weeks ago

fogrye commented 2 months ago

Hello,

First, thank you for your awesome library. We have integrated it into our project, graphiqlite. However, we have encountered a performance regression issue as described in this issue.

Issue Description

In our use case, we utilize the inNamespace() method to filter the classes we need. From my understanding, ComposerFinder iterates through all available classes to check if any meet the specified condition. This approach results in performance bottlenecks.

Proposed Optimization

To optimize this process, I suggest that the filter should first check the namespaces of dependencies. If a dependency's namespace does not match the inNamespace condition, then the classes within that dependency should not be checked at all.

For example, if we need to build a condition for inNamespace('\Symfony') combined with implementationOf('CachingInterface'), it would be efficient to:

  1. Check only the dependencies within the \Symfony namespace.
  2. Filter out classes that do not implement CachingInterface.

This optimization should significantly reduce unnecessary checks and improve performance.

Would this proposed optimization make sense for you? I appreciate any feedback you can provide.

Thank you!

alekitto commented 2 months ago

Hi @fogrye, Unfortunately, this is a little more complex than that.

First of all, I think this problem is present only if using a non-optimized classmap, so it should not be present in prod envs, as the filtering against a static array keys is very quick.

Talking about dev envs: there's no such thing as "dependency" in what composer generates, the only thing I have is a list of psr prefixes which are already filtered by this check.

Apart from the specific solution I've written about on the linked issue, a possible solution is to implement a cache as proposed in #20 via a custom file-finder object. Unfortunately, I did not have time to work on it lately, so it has been kind of stale, while I'm busy on other projects. If you want to tackle it, feel free to open a PR.

faizanakram99 commented 2 months ago

Hi @fogrye, Unfortunately, this is a little more complex than that.

First of all, I think this problem is present only if using a non-optimized classmap, so it should not be present in prod envs, as the filtering against a static array keys is very quick.

Not really, the problem is also with optimized autoloader (as you can see in the linked issue, classmap is already populated)

alekitto commented 2 months ago

Not really, the problem is also with optimized autoloader (as you can see in the linked issue, classmap is already populated)

Sorry, you posted a profile with a backtrace that clearly does not use an autoloader with authoritative classmap (searchInPsrMap should return immediately and should not yield any value). Can you double check that the composer-generated autoloader contains an authoritative classmap?

faizanakram99 commented 2 months ago

Not really, the problem is also with optimized autoloader (as you can see in the linked issue, classmap is already populated)

Sorry, you posted a profile with a backtrace that clearly does not use an autoloader with authoritative classmap (searchInPsrMap should return immediately and should not yield any value). Can you double check that the composer-generated autoloader contains an authoritative classmap?

My bad, I thought you meant optimize-autoloader, with classmap-authoritative it in indeed faster, still takes an extra second but that is okay (as compared to 45 seconds).

The classmap is already generated though (because of optimize autoloader) (as shown here as well https://github.com/thecodingmachine/graphqlite-bundle/pull/203#issuecomment-2139401623), the look up in filesystem is redundant here (at least for our case since all our classes do exist in class map).

classmap-authoritative is not suited during development though, maybe in additition to authoritative class it should also skip searchInPsrMap based on some flag, I see that the iterator accepts flags argument.

alekitto commented 2 months ago

with classmap-authoritative it in indeed faster, still takes an extra second but that is okay (as compared to 45 seconds).

Ok, that's a good starting point, as it signals that the bottleneck is the filesystem (and probably the issue is different based on the filesystem used, ie: docker mount on macos is the worst possible case).

it should also skip searchInPsrMap based on some flag

I could explore this in the next days, but I sincerely prefer implementing a cache as described in #20: generated classmap could be incomplete, and anyway the Psr*Iterators already skip classes already loaded from classmap, so I think your problem is a little bit deeper than we think.

alekitto commented 2 weeks ago

0.5.1 has been released with finder cache and ClassMapFinder I'm closing this, but feel free to re-open if still relevant.