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

[Config] fix perf of glob discovery when GLOB_BRACE is not available #35015

Merged
merged 1 commit into from Dec 18, 2019

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 17, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #35009
License MIT
Doc PR -

This PR implements a fast fallback implementation of GLOB_BRACE for musl-based libc, as found on Alpine. It is not a feature-complete fallback implementation. Implementing one would be much more involving. But the provided implementation is good enough in practice IMHO, and the slow path is still used when not-covered glob patterns are used.

Here is the comparison:
image

image

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Dec 17, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:config-glob-perf branch 2 times, most recently from d64b2a0 to 0526524 Dec 17, 2019
@nicolas-grekas nicolas-grekas changed the title [Config] improve perf of glob discovery when GLOB_BRACE is not available [Config] fix perf of glob discovery when GLOB_BRACE is not available Dec 17, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:config-glob-perf branch from 0526524 to b50c36a Dec 17, 2019
@stof

This comment has been minimized.

Copy link
Member

stof commented Dec 18, 2019

I think we need more tests covering the fallback usage, especially given we don't run our testsuite on Alpine

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 18, 2019

@stof can you please hint me about which kind of tests are missing to you?

@stof

This comment has been minimized.

Copy link
Member

stof commented Dec 18, 2019

Well, exercising it with a bunch of different globs, including cases which are unsupported by the fallback, to be sure they get properly detected as needing Finder rather than trying to be handled by the fallback implementation.

I was hoping that we could get inspiration from the test cases of the zend-stdlib implementation, but they seem to test only 1 case in their provider.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:config-glob-perf branch from b50c36a to 019a251 Dec 18, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 18, 2019

including cases which are unsupported by the fallback

I've made the fallback support nested braces instead :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:config-glob-perf branch from 019a251 to 8aad0e7 Dec 18, 2019
@stof

This comment has been minimized.

Copy link
Member

stof commented Dec 18, 2019

I've made the fallback support nested braces instead :)

you still don't have tests which include nested braces actually needing expansion.

and you are also missing tests covering what happens when feeding it with invalid braces for the glob.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Dec 18, 2019

Here you are.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:config-glob-perf branch from 8aad0e7 to 8af6d86 Dec 18, 2019
$p->setAccessible(true);
$p->setValue($resource, 0);

$this->assertSame([], array_keys(iterator_to_array($resource)));

This comment has been minimized.

Copy link
@stof

stof Dec 18, 2019

Member

is this actually what happens with glob, or does it report an error ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 18, 2019

Author Member

no error, just no matches

nicolas-grekas added a commit that referenced this pull request Dec 18, 2019
… available (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Config] fix perf of glob discovery when GLOB_BRACE is not available

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35009
| License       | MIT
| Doc PR        | -

This PR implements a fast fallback implementation of GLOB_BRACE for musl-based libc, as found on Alpine. It is *not* a feature-complete fallback implementation. Implementing one would be [much more involving](https://github.com/zendframework/zend-stdlib/blob/master/src/Glob.php). But the provided implementation is good enough in practice IMHO, and the slow path is still used when not-covered glob patterns are used.

Here is the comparison:
![image](https://user-images.githubusercontent.com/243674/71022909-eb9f7000-2101-11ea-99f5-eab0286c77a3.png)

![image](https://user-images.githubusercontent.com/243674/71022899-e4786200-2101-11ea-8663-80c1674602db.png)

Commits
-------

8af6d86 [Config] improve perf of glob discovery when GLOB_BRACE is not available
@nicolas-grekas nicolas-grekas merged commit 8af6d86 into symfony:4.4 Dec 18, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:config-glob-perf branch Dec 18, 2019
This was referenced Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.