WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 2 years ago

#27122 new enhancement

Optimization for PHP FPM

Reported by: dunglas Owned by:
Milestone: Future Release 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)

functions.diff (431 bytes) - added by dunglas 4 years ago.
wp-includes/functions.php patch
default-filters.diff (1.5 KB) - added by dunglas 4 years ago.
Updated patch
shutdown_flush.diff (1.6 KB) - added by dunglas 4 years ago.

Download all attachments as: .zip

Change History (20)

@dunglas
4 years ago

wp-includes/functions.php patch

#1 follow-up: @dd32
4 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 @nacin
4 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 @dunglas
4 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 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?

#4 @Denis-de-Bernardy
4 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.

#5 @SergeyBiryukov
4 years ago

  • Version changed from trunk to 3.8

#6 @Denis-de-Bernardy
4 years ago

Nevermind my last comment: I just verified that the globals are still around. They still are in PHP 5.3.

@dunglas
4 years ago

Updated patch

#7 @dunglas
4 years ago

The new patch is attached (default-filters.diff).

#8 follow-up: @Denis-de-Bernardy
4 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 @dunglas
4 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: @dunglas
4 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 @Denis-de-Bernardy
4 years ago

Replying to dunglas:

This new patch ( shutdown_flush.diff ) includes @Denis-de-Bernardy improvements but calls wp_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 @dunglas
4 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.

#13 @Denis-de-Bernardy
4 years ago

  • Keywords dev-feedback added

#14 @wonderboymusic
3 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release

#15 @chriscct7
2 years ago

  • Keywords needs-patch added; has-patch removed

#16 @lkraav
2 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.

#17 @lkraav
2 years ago

Adding a test mu-plugin with add_action "shutdown" fastcgi_finish_request() (<- pseudocode) made my multisite behave completely haywire. There may be more than meets the eye here.

Note: See TracTickets for help on using tickets.