Skip to content

[Finder] added scanner adapter #8685

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

Closed
wants to merge 7 commits into from

Conversation

jfsimon
Copy link
Contributor

@jfsimon jfsimon commented Aug 7, 2013

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
License MIT
Tests pass? yes

This PR improves finder performance using a pure PHP adapter.

This adapter:

  • only improve tests against the filename/path
  • don't visit children of excluded directories
  • tries to make the more calculations outside the loops

Benchmark:


NB: I grouped all new classes in a single Scanner namespace.

  • Benchmark if () { return } vs $closure()
  • Add followLink() support and test it

@saro0h
Copy link
Contributor

saro0h commented Aug 7, 2013

Nice performances ! 👍

{
$this->excludedNames = $excludedNames;

$this->minDepthRespected = 0 === $minDepth
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like:

if (0 !== $minDepth) {
    $this->minDepthRespected = function ($depth) use ($minDepth) { return $depth >= $minDepth; }; 
}

and later on:

public function isMinDepthRespected($depth)
{
    if (!$this->minDepthRespected instanceof \Clousure) {
        return true;
    }

    return call_user_func_array($this->minDepthRespected, array($depth));
}

Or something similar, IMO looks more clear =) What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more readable yes, but it introduces a test inside isMinDepthRespected() method which is called inside a loop and will surely impact performences. Don't you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you to benchmark it. Is the overhead of the instanceof check bigger than the overhead of a function call to return true ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think a function call itself has a big overhead? I'll perform benchs on this because this pattern is used almost everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some overhead for method calls compared to doing the return directly.

And this overhead becomes much bigger when installing XDebug (which is why there is lots of benchmarks telling array_key_exists is far slower than isset, which is not a function call in PHP)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stloyd well, you're right, I performed following benchmark https://gist.github.com/jfsimon/33480ec6ade7820bb313 and your solution is better than mine! I update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And call_user_func is slightly fester than call_user_func_array (~ 115 vs 125).

@jfsimon
Copy link
Contributor Author

jfsimon commented Aug 9, 2013

@stloyd @stof I updated the Constraints class (jfsimon@71dc01f), but perfs are not improved (https://gist.github.com/jfsimon/f58d6dda383be9f1bfdd#file-after-update), can you see why?

@fabpot
Copy link
Member

fabpot commented Dec 31, 2013

@jfsimon What's the status of this PR? I think getting a faster PHP implementation for the finder would be nice as I would really like to get rid of all those adapters.


private static function stringToRegex($value)
{
return '~(^|/)'.preg_quote($value, '#').'(/|$)~';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second argument to preg_quote() should match the delimiter being used ie ~.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks!

@fabpot
Copy link
Member

fabpot commented Mar 3, 2014

@jfsimon Any news on this one?

@jfsimon
Copy link
Contributor Author

jfsimon commented Mar 3, 2014

@fabpot let talk about this next friday?

@jfsimon jfsimon changed the title [POC] Finder: added scanner adapter [wip] Finder: added scanner adapter May 15, 2014
@jfsimon jfsimon changed the title [wip] Finder: added scanner adapter Finder: added scanner adapter May 16, 2014
@jfsimon jfsimon changed the title Finder: added scanner adapter [Finder] added scanner adapter May 16, 2014
@jfsimon
Copy link
Contributor Author

jfsimon commented May 16, 2014

@fabpot this PR is ready! Another one removing adapters will be ready in few hours.

@stof
Copy link
Member

stof commented May 16, 2014

Can we really remove adapters ? This would be a BC break

@stof
Copy link
Member

stof commented May 16, 2014

The registration if the adapter in the Finder (so that it is used) is missing

@jfsimon
Copy link
Contributor Author

jfsimon commented May 16, 2014

@stof thanks. I know removing adapters is a huge BC break, maybe it should done for the next Symfony major version? I submit the PR anyway...

@stof
Copy link
Member

stof commented May 16, 2014

@jfsimon it can only be done in the next major version. They could be deprecated though

@OskarStark
Copy link
Contributor

any news on this PR?

the work looks great but you should look on fabbot and the failing travis @jfsimon

@fabpot
Copy link
Member

fabpot commented Sep 15, 2015

Closing in favor of #15802

@fabpot fabpot closed this Sep 15, 2015
fabpot added a commit that referenced this pull request Sep 15, 2015
…t to skip looping over excluded directories (nicolas-grekas)

This PR was submitted for the 2.8 branch but it was merged into the 2.3 branch instead (closes #15802).

Discussion
----------

[Finder] Handle filtering of recursive iterators and use it to skip looping over excluded directories

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5951, #8685
| License       | MIT
| Doc PR        | -

By implementing RecursiveIterator in FilterIterator, we can make it able to skip children of excluded branches of recursive inner iterators.
We use it immediately for our main target: skip over children of excluded directories in the Finder.
This is a significant performance boost when iterating over big directories, thus the "bugfix" status.

Commits
-------

8c691bd [Finder] Handle filtering of recursive iterators and use it to skip looping over excluded directories
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants