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

[performance] Symfony Kernel::boot() in dev mode on Alpine #35009

Closed
bastnic opened this issue Dec 17, 2019 · 17 comments
Closed

[performance] Symfony Kernel::boot() in dev mode on Alpine #35009

bastnic opened this issue Dec 17, 2019 · 17 comments

Comments

@bastnic
Copy link
Contributor

@bastnic bastnic commented Dec 17, 2019

This is a follow-up of this discussion.

Alpine is the default docker linux base distribution. It's widely used. It's based on musl libc, and so, it's quite different of traditional linux distribution as Debian. One the difference is that glob does not handle GLOB_BRACE.

I knew about it from an old issue on composer that we already experienced.

On a big project, we had quite castrophous boot time, and I finally checked on it.

The first thing is that Iterator/SortableIterator.php/44-46 cost an enormous amout of time and is called 22k time.

Searching why, It seems that we never enter is the optimized path here

if (0 !== strpos($this->prefix, 'phar://') && false === strpos($this->pattern, '/**/') && (\defined('GLOB_BRACE') || false === strpos($this->pattern, '{'))) {
.

I had that in Kernel::configureContainer:

const CONFIG_EXTS = '.{php,xml,yaml,yml}';
// ... 
$loader->load($confDir . '/{packages}/*' . self::CONFIG_EXTS, 'glob'); 
$loader->load($confDir . '/{packages}/' . $this->environment . '/**/*' . self::CONFIG_EXTS, 'glob');
$loader->load($confDir . '/{services}' . self::CONFIG_EXTS, 'glob');

and in services.yaml:

Lib\Core\:
    resource: '../src/*'
    exclude: '../src/{Entity,Migrations,Translation,Tests,Kernel.php,PHPStan,Fixtures,Traits}'

As I don't have GLOB_BRACE and every pattern given contains a brace, it always fallback on the symfony/finder implementation which is wayyy slower.

By removing the /**/*, and only searching into yaml extension, I expect to be in the fast path. But the brackets on {packages} and {services} prevents it. By removing them it goes into the fast path. And gives a clear boost.

$loader->load($confDir . '/packages/*' . self::CONFIG_EXTS, 'glob');

if (is_dir($confDir . '/packages/' . $this->environment)) {
	$loader->load($confDir . '/packages/' . $this->environment . '/*' . self::CONFIG_EXTS, 'glob');
}
$loader->load($confDir . '/services.yaml');

I didn't see any drwabacks (and I have a very extensive tests suites).

Rest the case with multiple option separated by comma in brackets, that stills cost a lot.

I'm absolutly thinks this is a serious issue, but I'm not sure of what todo with it, hance the absence of PR.

  • brackets are they really necessary for packages and services? (edit answer: it ONLY ensure that the file / dir is optional. On an app that is older than one day, I'm sure of the structure that I use, and it should be totally recommended in perf reco to update that Kernel stuff to be exact and don't let it guess.)
  • should we purpose a better implementation on that hot path than the symfony/finder one. @nicolas-grekas purpose to "we could have some logic that splits a single glob-with-braces into several of them" symfony/recipes#705 (comment)
@lyrixx lyrixx changed the title [performance] Symfony Kernel::boot in dev mode on Alpine [performance] Symfony Kernel::boot() in dev mode on Alpine Dec 17, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 17, 2019

brackets are they really necessary for packages and services?

Yes, I explained why in symfony/recipes#705 (comment)
we could remove some of them, but that would make the defaults more rigid - ie favoring perf against DX. We can do better.

"we could have some logic that splits a single glob-with-braces into several of them"

I confirm this one, it shouldn't be that hard.

@stof

This comment has been minimized.

Copy link
Member

@stof stof commented Dec 17, 2019

I'm transferring this issue to the symfony repo, as that's not specifically about recipes.

@stof stof transferred this issue from symfony/recipes Dec 17, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 17, 2019

About my suggestion to split single glob-with-braces, here is the description from the gnu doc:
https://www.gnu.org/software/libc/manual/html_node/More-Flags-for-Globbing.html

@lyrixx lyrixx added the Performance label Dec 17, 2019
@stof stof added the Config label Dec 17, 2019
@nicolas-grekas nicolas-grekas added this to Nicolas' in Help Wanted Dec 17, 2019
@bastnic

This comment has been minimized.

Copy link
Contributor Author

@bastnic bastnic commented Dec 17, 2019

I tries with this implementation, and I get a stunning optimization of -212ms... (-64%)

https://blackfire.io/profiles/compare/3ff797c4-a931-4f18-8b4e-a57982d3d2cd/graph

image

nicolas-grekas added a commit that referenced this issue 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
@bastnic

This comment has been minimized.

Copy link
Contributor Author

@bastnic bastnic commented Dec 18, 2019

Thanks a lot @nicolas-grekas!

@Sh1d0w

This comment has been minimized.

Copy link

@Sh1d0w Sh1d0w commented Dec 19, 2019

@nicolas-grekas I think your fix broke the discovery of package configurations. If I apply your changes from (d7a0679) I get the following error:

The child node "defaults" at path "overblog_dataloader" must be configured.

If I remove the changes everything works fine.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 19, 2019

Can you please provide a reproducer?

@Sh1d0w

This comment has been minimized.

Copy link

@Sh1d0w Sh1d0w commented Dec 19, 2019

@nicolas-grekas Sorry, can't provide atm as the project we are working on is private and I am currently on a vacation.

The package that errors is https://packagist.org/packages/overblog/dataloader-bundle

We have the full configuration file set up with the defaults key as well.

We are running our app with Docker: FROM alpine:3.10.2

The error The child node "defaults" at path "overblog_dataloader" must be configured. is thrown only after I apply the changes from your commit, if I remove them everything works fine.

I will try to put something as POC these days, but in the meantime I think thats enough info to reproduce.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 19, 2019

@Sh1d0w I am able to reproduce the error, but it looks unrelated to this change. I still have the error with v4.4.1.

If I remove the changes everything works fine.

is this the only change you're doing or are there any others, like updating the bundle also?

@Sh1d0w

This comment has been minimized.

Copy link

@Sh1d0w Sh1d0w commented Dec 19, 2019

@nicolas-grekas I've upgraded from Symfony 4.3 to 4.4 .

The bundle that errors is using the same version v0.4.2 and I did not update its version.

After the update to Symfony 4.4 I tested the app and it worked fine. Then tried to apply the change from your commit as I was keen to try it out before it was published, and I got this error. I've removed the changes to the GlobResource everything works fine.

Just to be clear if I revert to Symfony 4.3 where everything is working ok as well, and apply the same changes to GlobResource I get the same error.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 19, 2019

Can you share the output of print_r($paths) just before this line, which exists in both versions?
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Config/Resource/GlobResource.php#L118

@Sh1d0w

This comment has been minimized.

Copy link

@Sh1d0w Sh1d0w commented Dec 19, 2019

@nicolas-grekas Actually your fix is working fine, the problem was on my end! Really sorry for the spam and thanks for the great fix :)

@bastnic

This comment has been minimized.

Copy link
Contributor Author

@bastnic bastnic commented Dec 19, 2019

@Sh1d0w good news. Can you share the perf improvement on your side? It's very impressive on my project but we miss other feedback.

@Sh1d0w

This comment has been minimized.

Copy link

@Sh1d0w Sh1d0w commented Dec 19, 2019

Sure, it resulted approximately 50% reduction in load times, which is more than great (previously ~700ms, after the fix ~300 ms)

@bastnic

This comment has been minimized.

Copy link
Contributor Author

@bastnic bastnic commented Dec 19, 2019

@Sh1d0w wooot!

@Sh1d0w

This comment has been minimized.

Copy link

@Sh1d0w Sh1d0w commented Dec 19, 2019

@nicolas-grekas Will this fix be back ported and released for older versions of Symfony as a patch, or will be available only for 4.4 and above?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 19, 2019

@Sh1d0w that's going to be for 4.4 and up only. One more reason to upgrade asap :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Help Wanted
Nicolas'
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.