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

Room for optimisation for ComposerFinder #21

Closed
fogrye opened this issue May 31, 2024 · 6 comments
Closed

Room for optimisation for ComposerFinder #21

fogrye opened this issue May 31, 2024 · 6 comments

Comments

@fogrye
Copy link

fogrye commented May 31, 2024

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
Copy link
Owner

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
Copy link

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
Copy link
Owner

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
Copy link

faizanakram99 commented May 31, 2024

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 thecodingmachine/graphqlite-bundle#203 (comment)), 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
Copy link
Owner

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
Copy link
Owner

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

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

No branches or pull requests

3 participants