#55069 closed enhancement (fixed)
Optimize fread() calls using file_get_contents() or stream_get_contents()
Reported by: | maxkellermann | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.0 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | has-patch |
Focuses: | performance | Cc: |
Description (last modified by )
The POMO_FileReader::read_all() method uses fread() with 4 kB blocks, but stream_get_contents() would be much faster.
Attachments (8)
Change History (28)
This ticket was mentioned in PR #2267 on WordPress/wordpress-develop by MaxKellermann.
3 years ago
#1
- Keywords has-patch added
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
#2
@
3 years ago
- Focuses performance added
- Milestone changed from Awaiting Review to 6.0
#3
@
3 years ago
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.
#5
@
3 years ago
Thanks for the patch @maxkellermann . Do you have some before and after numbers on the performance improvement?
#6
@
3 years ago
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.
#12
@
3 years ago
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 theis_dir()
andis_readable()
PHP functions, respectively, with the error suppression operator (@
). The subsequentdir()
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 toopen_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?
#13
follow-up:
↓ 14
@
3 years ago
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.
#14
in reply to:
↑ 13
;
follow-up:
↓ 15
@
3 years ago
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.
#15
in reply to:
↑ 14
@
3 years ago
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.
#16
@
3 years ago
- Component changed from I18N to General
- Description modified (diff)
- Summary changed from Optimize POMO_FileReader.read_all() using stream_get_contents() to Optimize fread() calls using file_get_contents() or stream_get_contents()
Just renaming the ticket as the focus has moved to other parts of core as well.
#17
@
3 years ago
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.
#19
@
2 years ago
- Milestone changed from 6.1 to 6.0
- Resolution set to fixed
- Status changed from accepted to closed
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!
SergeyBiryukov commented on PR #2267:
2 years ago
#20
Thanks for the PR! Merged in https://core.trac.wordpress.org/log/?revs=52696,52698,52701,52718.
Suggested patch