Open swissspidy opened 3 months ago
Thanks for opening this @swissspidy! +1 on brainstorming here which additional performance related checks could be valuable. cc @mukeshpanchal27 @joemcgill @westonruter @adamsilverstein
Sharing further ideas here, some of which I dug up from an earlier design exploration from 1.5 years ago:
Non_Blocking_Scripts_Check
: Would warn about enqueued scripts that use neither defer
nor async
. Potentially we could also accept a blocking script as long as it's $in_footer
. But it would definitely warn about a blocking head
script.
Performant_WP_Query_Params_Check
: Would warn about problematic WP_Query
parameter usage, like the slow post__not_in
or posts_per_page => -1
or cache_results => false
, for example.
WordPressVIPMinimum.Performance.WPQueryParams
and WordPress.DB.SlowDBQuery
.Uncached_DB_Queries_Check
: Would warn about any direct SELECT
database queries which are not "wrapped" in (or rather "surrounded by") wp_cache_*()
or transient functions.
Image_Functions_Usage_Check
: Would warn about any direct output of img
tags in PHP code and templates, as those should use wp_get_attachment_image()
etc. instead, which comes with performance benefits. The exception is img
tags which aren't for attachments. For such manual img
tags, the check could trigger a warning if the wp_get_loading_optimization_attributes()
is not used as part of generating the output.
Non_Blocking_Scripts_Check
: Would warn about enqueued scripts that use neitherdefer
norasync
. Potentially we could also accept a blocking script as long as it's$in_footer
. But it would definitely warn about a blockinghead
script.
- Probably would be a static analysis check, though maybe implementing as a runtime check would have benefits. Worth exploring.
The corresponding PHPCS sniff does not yet support the new $args
param we added in 6.3. It was also indicated that the sniff might actually be removed because it's harder to detect. So probably needs to be a runtime check.
Performant_WP_Query_Params_Check
: Would warn about problematicWP_Query
parameter usage, like the slowpost__not_in
orposts_per_page => -1
orcache_results => false
, for example.
- Probably would be a PHPCodeSniffer analysis check, using e.g.
WordPressVIPMinimum.Performance.WPQueryParams
andWordPress.DB.SlowDBQuery
.
We already have Performant_WP_Query_Params_Check
which uses these two sniffs 🤔
Uncached_DB_Queries_Check
: Would warn about any directSELECT
database queries which are not "wrapped" in (or rather "surrounded by")wp_cache_*()
or transient functions.
- This may be tricky to automate, but worth exploring. Unsure whether a static check could achieve that, we probably need a runtime check, or maybe even some hybrid, which first scans the code for direct DB queries and then executes the respective functions to check whether the query is being cached.
I'd say this is impossible with static analysis, caching & query parts are not always co-located. So would need a runtime check.
Could even be as simple as this:
Downside:
go_to
to set the global WP_Query
. Easy to miss stuff this way.
Image_Functions_Usage_Check
: Would warn about any direct output ofimg
tags in PHP code and templates, as those should usewp_get_attachment_image()
etc. instead, which comes with performance benefits. The exception isimg
tags which aren't for attachments. For such manualimg
tags, the check could trigger a warning if thewp_get_loading_optimization_attributes()
is not used as part of generating the output.
- Probably would be a static check.
Hmm I could swear there was a ticket for this somewhere or even an existing sniff, but can't find anything right now 🤔
For a static check we'd need someone who's good at writing PHPCS sniffs.
Some more performance sniffs from https://github.com/Automattic/VIP-Coding-Standards we're not currently using:
CacheValueOverrideSniff
wp_cache_get()
. See https://github.com/Automattic/VIP-Coding-Standards/blob/868767496ed0ce92199233811fa5b35a50800145/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.incFetchingRemoteDataSniff
file_get_contents()
usage (even if it's for an actual file) and recommends using wpcom_vip_file_get_contents()
instead.LowExpiryCacheTimeSniff
NoPagingSniff
nopaging
to WP_Query
. Or well, actually it throws if nopaging
is found in any array — so lots of false positives. Don't think it's useful.NoPagingSniff
OrderByRandSniff
orderby=>rand
when it finds that in any array. Quite the edge case IMHO, not really worth adding.RegexpCompareSniff
RemoteRequestTimeoutSniff
TaxonomyMetaInOptionsSniff
I'm going to work on breaking out issues to further explore these ideas. I started with https://github.com/WordPress/plugin-check/issues/467
@swissspidy what do you think about adding a performance
label to make tracking these issues a bit easier?
RemoteRequestTimeoutSniff Flag use of a timeout of more than 3 seconds for remote requests.
@swissspidy do you think this one is worth adding? I'm sure there are legitimate use cases for doing this, eg. slow API. Still not a good idea generally so maybe worth a warning?
We could add it, but not with a high priority. Such requests are usually cached or running in background jobs, so real user impact is low. And overall it's not super common I'd say. So such a check wouldn't have the biggest impact IMO.
Right now the following performance-related checks or enhancements are proposed:
19
23
74
28
Ideally we'd have many more of those, so I am opening this issue for us to do some brainstorming.