Optimize fread() calls using file_get_contents() or stream_get_contents()

[maxkellermann]

The POMO_FileReader::read_all() method uses fread() with 4 kB blocks, but stream_get_contents() would be much faster.

[maxkellermann]

Suggested patch

stream_get_contents() is faster, because the PHP core can decide how
to best read the reamining file; it could decide to issue just one
read() call or mmap() the file first.

The old chunk size of 4 kB was rather clumsy, because it is smaller
than PHP's default stream buffer size of 8 kB.

Trac ticket: https://core.trac.wordpress.org/ticket/55069

[SergeyBiryukov]

Interesting, thanks!

Introduced in [12174] / #10165 for WordPress 2.9. At the time, WordPress still needed to support PHP 4.3+. I guess that was the reason for not using stream_get_contents() here, as it's only available in PHP 5+.

[maxkellermann]

I just submitted a related PR to the PHP project: ​https://github.com/php/php-src/pull/8032
This PHP PR optimizes the stream_get_contents() implementation to skip buffering in some cases, which allows slurping the whole file in just one read() system call instead of refilling the read buffer in 8 kB steps.

[maxkellermann]

I have added 4 more patches with micro-optimizations.

[johnbillion]

Thanks for the patch @maxkellermann . Do you have some before and after numbers on the performance improvement?

[maxkellermann]

No, I don't have numbers for these specific patches, but the improvements should be similar to the numbers I posted on ​https://github.com/php/php-src/pull/8032 because they allow WordPress to take advantage of these optimizations.

However, the WordPress improvement will be very small until my PHP patches are merged, because PHP's own internal functions are just as unoptimized as WordPress's custom implementation. Only the combination of the two is much faster.

[SergeyBiryukov]

In 52696:

Code Modernization: Use stream_get_contents() in POMO_FileReader::read_all().

stream_get_contents() is faster than fread(), because the PHP core can decide how to best read the remaining file; it could decide to issue just one read() call or mmap() the file first.

Per the PHP manual, file_get_contents() or stream_get_contents() is the preferred way to read the contents of a file into a string. It will use memory mapping techniques if supported by the OS to enhance performance.

Reference: ​PHP Manual: file_get_contents().

Follow-up to [12174].

Props maxkellermann.
See #55069.

[SergeyBiryukov]

In 52698:

Code Modernization: Use file_get_contents() in get_file_data().

file_get_contents() is faster than fread(), because the PHP core can decide how to best read the remaining file; it could decide to issue just one read() call or mmap() the file first.

Per the PHP manual, file_get_contents() or stream_get_contents() is the preferred way to read the contents of a file into a string. It will use memory mapping techniques if supported by the OS to enhance performance.

Reference: ​PHP Manual: file_get_contents().

Follow-up to [12044], [49073], [52696].

Props maxkellermann.
See #55069.

[SergeyBiryukov]

In 52701:

Code Modernization: Use file_get_contents() in wp_get_image_mime().

file_get_contents() is faster than fread(), because the PHP core can decide how to best read the remaining file; it could decide to issue just one read() call or mmap() the file first.

Per the PHP manual, file_get_contents() or stream_get_contents() is the preferred way to read the contents of a file into a string. It will use memory mapping techniques if supported by the OS to enhance performance.

Reference: ​PHP Manual: file_get_contents().

Follow-up to [50810], [52696], [52698].

Props maxkellermann.
See #55069.

[SergeyBiryukov]

In 52718:

Code Modernization: Use file_get_contents() in wp_get_webp_info().

file_get_contents() is faster than fread(), because the PHP core can decide how to best read the remaining file; it could decide to issue just one read() call or mmap() the file first.

Per the PHP manual, file_get_contents() or stream_get_contents() is the preferred way to read the contents of a file into a string. It will use memory mapping techniques if supported by the OS to enhance performance.

Reference: ​PHP Manual: file_get_contents().

Follow-up to [50810], [52696], [52698], [52701].

Props maxkellermann.
See #55069.

[SergeyBiryukov]

Thanks for the patches!

Looks like 0002-class-wp-filesystem-direct-remove-unnecessary-is_dir.patch​ is the only one left. My concerns here:

• The ::is_dir() and ::is_readable() methods call the is_dir() and is_readable() PHP functions, respectively, with the error suppression operator (@). The subsequent dir() call does not have it. If we remove these checks, would that lead to PHP warnings on some installs which had none previously, for example due to open_basedir restrictrion? Seems like this could use some testing.
• The WP_Filesystem_SSH2::dirlist() method has a similar block, does it need the same change?

[maxkellermann]

would that lead to PHP warnings on some installs which had none previously

You are right. It seems PHP people very much like the idea of sprinkling unnecessary access() and stat() system calls everywhere.

I'm not a PHP programmer; I do lower-level code, and one very simple idea to make code faster is to omit stuff that's not necessary. For example, what's the point of doing access() to check if a file exists before opening it; just open() it and then you can still handle the same ENOENT. Doing system calls has become more expensive recently due to the Meltdown/Spectre mitigations, and avoiding them does make a difference.

Additionally, the extra access()/stat() is buggy; it's a TOCTOU bug. It's not only slower, but also unreliable.

Knowing that, it is surprising that PHP punishes fast and reliable code with a warning, which is a very visible signal to their users: this software is bad. That's sad.

I can understand if you don't want to merge my remaining patch for that reason. But if you do want it, I'll investigate what else can be optimized that way and send more patches.

Maybe this kind of warning can be suppressed? I don't know, I'm not a PHP expert.

[SergeyBiryukov]

Replying to maxkellermann:

But if you do want it, I'll investigate what else can be optimized that way and send more patches.

Please do :)

Maybe this kind of warning can be suppressed?

Yes, on second thought, we can just add error suppression for the dir() call.

[SergeyBiryukov]

Replying to SergeyBiryukov:

Yes, on second thought, we can just add error suppression for the dir() call.

Hmm, it looks like is_readable() was added in [45611/trunk/src/wp-admin/includes/class-wp-filesystem-direct.php] specifically to remove error suppression from the dir() call, so the patch reverts that part of the changeset. It also causes a WPCS warning:

FILE: src\wp-admin\includes\class-wp-filesystem-direct.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 609 | WARNING | Silencing errors is strongly discouraged. Use proper
     |         | error checking instead. Found: @dir( $path ...
     |         | (WordPress.PHP.NoSilencedErrors.Discouraged)
----------------------------------------------------------------------

So we would need a // phpcs:ignore comment here.

See 0002-class-wp-filesystem-direct-remove-unnecessary-is_dir.2.patch​.

[SergeyBiryukov]

Just renaming the ticket as the focus has moved to other parts of core as well.

[peterwilsoncc]

Given it's a microoptimisation, I'd rather avoid additional sniff suppression and maintain the is_readable():

• if the sniff is wrong/too strict that ought to be discussed
• with recent PHP changes, avoiding the STHU operator seems wise

If anything it's probably the methods being called that needs to change but that's a discussion for another ticket.

[SergeyBiryukov]

Given that 0002-class-wp-filesystem-direct-remove-unnecessary-is_dir.2.patch​ is the only patch left here and it's not going to be addressed at this time per comment:17, closing this as fixed for 6.0.

Please create a new ticket for any follow-up changes. Thanks!

Thanks for the PR! Merged in https://core.trac.wordpress.org/log/?revs=52696,52698,52701,52718.