Opened 11 years ago
Last modified 7 days ago
#27122 assigned enhancement
Optimization for PHP FPM
Reported by: | dunglas | Owned by: | pbearne |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | 3.8 |
Component: | General | Keywords: | dev-feedback needs-patch |
Focuses: | performance | Cc: |
Description
This patch make wp_ob_end_flush_all
calling fastcgi_finish_request
instead of ob_end_flush
when php-fpm is used.
fastcgi_finish_request
flush the buffers and close the connection. This tweak increases page speed.
It also allows to run heavy tasks such as sending mail, writing logs or making complex calculations after the end of the request without slowing down the whole page load.
Symfony uses this tweak too, see this PR FOR further details: https://github.com/symfony/symfony/issues/1180
Attachments (3)
Change History (21)
#1
follow-up:
↓ 3
@
11 years ago
I don't think this is a safe optimization to make unfortunately.
Consider the scenario where wp_ob_end_flush_all()
is called mid-pageload by plugins for example, which we also do in core:
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/misc.php#L290
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-upgrader-skins.php#L251
Perhaps a safer option, would be to conditionally add fastcgi_finish_request
to the shutdown
action at priority 2? as you can see wp_ob_end_flush_all ()
being used here:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/default-filters.php#L250
What do you think @dunglas?
#2
@
11 years ago
Yeah, we wouldn't want this in wp_ob_end_flush_all(), but having it on shutdown before priority 10 seems fine to me.
#3
in reply to:
↑ 1
@
11 years ago
Sorry, I was not aware that this function is used in other places.
I agree with your solution @dd32 except that this is better in a performance point of view to call fastcgi_finish_request()
before the last call to ob_end_flush()
and it's even better to not call it at all like in the Symfony implementation.
A compromise can be to register fastcgi_finish_request()
to the shutdown hook with a priority of 1 if the function exists and (always if the function exists) to not register wp_ob_end_flush_all()
to this hook.
This will maximizes performances on FPM and will not break existing code because the behavior of wp_ob_end_flush_all()
is not modified.
If you think this solution is fine I'll submit a new patch tomorrow.
Replying to dd32:
I don't think this is a safe optimization to make unfortunately.
Consider the scenario where
wp_ob_end_flush_all()
is called mid-pageload by plugins for example, which we also do in core:
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/misc.php#L290
https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/class-wp-upgrader-skins.php#L251
Perhaps a safer option, would be to conditionally add
fastcgi_finish_request
to theshutdown
action at priority 2? as you can seewp_ob_end_flush_all ()
being used here:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/default-filters.php#L250
What do you think @dunglas?
#4
@
11 years ago
The reason that function is on the shutdown hook was that (in prior PHP versions at least) PHP would trash the various WP globals prior to calling output buffer-related callback functions.
I'd advise to check if, with this function on the shutdown hook at priority 10, you can still see e.g. $wpdb
at priority 11. If not, adding the call will potentially break plugins that hook after it.
#6
@
11 years ago
Nevermind my last comment: I just verified that the globals are still around. They still are in PHP 5.3.
#8
follow-up:
↓ 9
@
11 years ago
I don't like this patch. It makes it harder than it needs if you ever need or want to remove the flush function.
Can I suggest the following instead?
function wp_finish_request() { if (!WP_DEBUG && function_exists('fastcgi_finish_request')) { fastcgi_finish_request(); } } add_action('shutdown', 'wp_finish_request', 2);
That function would need a docblock, too. The point is, it shouldn't be enabled when debugging, and a plugin should be able to remove it without worrying about its implementation details.
#9
in reply to:
↑ 8
@
11 years ago
I agree to disable this optimization in debug mode, however the problem with your proposal is that wp_ob_end_flush_all()
is always called even when this is unnecessary.
I'll provide a new batch ASAP.
Replying to Denis-de-Bernardy:
I don't like this patch. It makes it harder than it needs if you ever need or want to remove the flush function.
Can I suggest the following instead?
function wp_finish_request() { if (!WP_DEBUG && function_exists('fastcgi_finish_request')) { fastcgi_finish_request(); } } add_action('shutdown', 'wp_finish_request', 2);That function would need a docblock, too. The point is, it shouldn't be enabled when debugging, and a plugin should be able to remove it without worrying about its implementation details.
#10
follow-up:
↓ 11
@
11 years ago
This new patch ( shutdown_flush.diff
) includes @Denis-de-Bernardy improvements but calls wp_ob_end_flush_all()
only if necessary.
#11
in reply to:
↑ 10
@
11 years ago
Replying to dunglas:
This new patch (
shutdown_flush.diff
) includes @Denis-de-Bernardy improvements but callswp_ob_end_flush_all()
only if necessary.
I think you're misunderstanding or misinterpreting what wp_ob_end_flush_all()
was initially about.
The function was introduced due to a change in the way PHP handles object destruction around 5.1 or 5.2: global variables that were garbage collected after all code had run suddenly began to get garbage collected prior to calling output buffer callbacks. This triggered fatal errors as a result, and to the best of my knowledge the behavior has remained unchanged since.
See #3354 and this (rejected) PHP bug I reported back then:
https://bugs.php.net/bug.php?id=40104
Therefor my suggestion for the more conservative approach, i.e. a completely separate and independent function, that addresses the php-fpm optimization and nothing else.
#12
@
11 years ago
You're right I don't understand the relation between the PHP bug you mention and my patch.
fastcgi_finish_request()
automatically flushes data in buffers, so the last call to wp_ob_end_flush_all()
is not necessary.
But if you think this patch can cause unexpected behaviors I'm fine with the "conservative" approach.
#14
@
10 years ago
- Keywords has-patch added
- Milestone changed from Awaiting Review to Future Release
#16
@
9 years ago
What's the latest understanding about this? Seems like a good thing to have. "has-patch" keyword was dropped, but with no comments what specifically should be addressed since 2 years have gone by.
wp-includes/functions.php patch