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

[DI] generate preload.php file for PHP 7.4 in cache folder #32032

Open
wants to merge 1 commit into
base: 4.4
from

Conversation

@nicolas-grekas
Copy link
Member

commented Jun 13, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29105
License MIT
Doc PR -

This PR makes the PhpDumper generate a preloading file suited for PHP 7.4.
On a skeleton app, the generated file is var/cache/dev/srcApp_KernelDevDebugContainer.preload.php (of course, this varies by env name + kernel class)

One missing thing is listing some classes that are always needed but are not related to services.

Typically: Request and Response. We might need a new mechanism to make this list extensible.

I did not measure the benefit of this on PHP 7.4. I would really appreciate if someone could give it a try on PHP 7.4 with preloading enabled.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:preload branch 2 times, most recently from 85e1fec to e2b7dce Jun 13, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

For reference, here is some previous work by @BenMorel that shows that loading only the hot classes (vs everything) is what provides the best boost. Identifying these hot classes is what I'm trying to achieve here.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Is it really worth it to preload only hot classes? Doesn't that increase the complexity a lot, vs just preloading everything?

@BenMorel

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

There's a slight performance improvement vs preloading everything when preloading hot classes that were selected using actual runtime data, as reported by opcache after a few runs of the application:

Benchmark Requests per second Diff
No preloading 631 rq/s -
Preload hot classes 738 rq/s +17%
Preload everything 712 rq/s +13%

(original benchmark in this thread)

Any other pre-computed list would need to be benchmarked again, as the results may be very different (and could even be slower than preloading everything, as the list may be missing classes that will need to be linked at runtime).

All in all, the difference between a carefully optimized preloader and an eager preloader is small, I'd personally be in favour of preloading everything by default, and let users create their own preloading script if they need the extra few % of performance.

What could be interesting however, is shipping Symfony with a tool (like the one I used in this post) to generate the preloading file from opcache, together with some instructions on how to:

  • restart the server
  • navigate the website / use as many features of the app as possible
  • then dump the preloading script
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Identifying the hot classes using runtime data from opcache has a big drawback: rebooting the fpm pool on prod servers. Not sure it's calling for good ops practices.
Without having working numbers, it's way too early to draw any conclusions or close any doors.
You may be right though, but please allow me to do the experiment :)

@BenMorel

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

I did not have in mind to gather runtime data from the prod environment, but from the dev one!

No worries, I just wanted to give a bit of warning here, I'm looking forward to your numbers!

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:preload branch 4 times, most recently from 5deabb4 to aa39efe Jun 15, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:preload branch from aa39efe to 8aa4aca Jun 17, 2019

@k00ni

This comment has been minimized.

Copy link

commented Jun 18, 2019

I am not sure, if this is the right place to ask.

In the related RFC they say:

The traded-in flexibility would include the inability to update these files once the server has been started (updating these files on the filesystem will not do anything; A server restart will be required to apply the changes); And also, this approach will not be compatible with servers that host multiple applications, or multiple versions of applications [...]

How do you plan to manage updates in the vendor folder, if you have to restart the server to apply changes? If am on a shared hosting environment, would that affect me when using Symfony in the future?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

Preloading is fundamentally incompatible with shared hosting yes, so you wouldn't use this on your provider. The added file is totally opt-in so one won't use it if one isn't able to restart the FPM server when deploying new code, that's correct.

@BenMorel

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

If am on a shared hosting environment, would that affect me when using Symfony in the future?

Nope, preloading requires setup at server level (opcache.preload), which will never be enabled on shared hosting environments.

Most likely any preloading script shipped with Symfony or Composer will be opt-in, and the choice to include it or not will be left to the server administrator.

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

It's 2019, so hopefully the population of developers who unfortunately still have to deal with shared hosting is shrinking rapidly...

fabpot added a commit that referenced this pull request Jun 20, 2019

bug #32110 Revert "minor #32054 Prepare for PHP 7.4 preload (nicolas-…
…grekas)" (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

Revert "minor #32054 Prepare for PHP 7.4 preload (nicolas-grekas)"

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #32032
| License       | MIT
| Doc PR        | -

This reverts commit a0aa941, reversing
changes made to 8496003.

This change is incompatible with Composer's class map generation.

Commits
-------

8a1813a Revert "minor #32054 Prepare for PHP 7.4 preload (nicolas-grekas)"
@tsantos84

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Hi @nicolas-grekas, if we have this feature in composer we could configure the classes directly on composer.json file and it could work for vendor/package level as well. Makes sense?

HttpFoundation composer.json file:

{
    "preload": [
         "Symfony\\HttpFoundation\\{Request,Response}",
         "Symfony\\EntireNamespace\\*",
         "Symfony\\{NS1,NS2}\\*"
    ]
}
@tsantos84

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

I've just read the RFC at composer/composer#7777 and seems that is not an easy decision to put such feature on composer.

@arheyy

arheyy approved these changes Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.