Add more performance-related checks

[swissspidy]

Right now the following performance-related checks or enhancements are proposed:

• #19
• #23
• #74
• #28
• #467

Ideally we'd have many more of those, so I am opening this issue for us to do some brainstorming.

[felixarntz]

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.
  • Probably would be a static analysis check, though maybe implementing as a runtime check would have benefits. Worth exploring.
• 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.
  • Probably would be a PHPCodeSniffer analysis check, using e.g. 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.
  • 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.
• 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.
  • Probably would be a static check.

[swissspidy]

• 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.

  • 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 problematic WP_Query parameter usage, like the slow post__not_in or posts_per_page => -1 or cache_results => false, for example.

  • Probably would be a PHPCodeSniffer analysis check, using e.g. WordPressVIPMinimum.Performance.WPQueryParams and WordPress.DB.SlowDBQuery.

We already have Performant_WP_Query_Params_Check which uses these two sniffs 🤔

• 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.

  • 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:

1. Perform runtime check without any plugin, count number of db queries
2. Perform check again with the plugin, see if the count is different
3. If yes, perform the check again, and count again. If the count is not the original again, then you know there is some uncached query

Downside:

• Not very accurate
• Don't necessarily know which file & line the query is coming from (though a hybrid approach could help)
• Our runtime checks are not very smart, they just use go_to to set the global WP_Query. Easy to miss stuff this way.

• 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.

  • 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
  • Useless IMHO. Just checks whether you're accidentally overriding a value again that you just retrieved via wp_cache_get(). See https://github.com/Automattic/VIP-Coding-Standards/blob/868767496ed0ce92199233811fa5b35a50800145/WordPressVIPMinimum/Tests/Performance/CacheValueOverrideUnitTest.inc
• FetchingRemoteDataSniff
  • Complains about any file_get_contents() usage (even if it's for an actual file) and recommends using wpcom_vip_file_get_contents() instead.
  • If we want a check about remote data fetching we should probably do one that checks whether remote requests are cached (like the proposed db check above)
• LowExpiryCacheTimeSniff
  • Simply throws a warning when low cache times are set. Not sure how useful it is in practice.
• NoPagingSniff
  • Warns if passing 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
  • Prevents orderby=>rand when it finds that in any array. Quite the edge case IMHO, not really worth adding.
• RegexpCompareSniff
  • Flag REGEXP and NOT REGEXP in meta compare. Ditto here.
• RemoteRequestTimeoutSniff
  • Flag use of a timeout of more than 3 seconds for remote requests.
• TaxonomyMetaInOptionsSniff
  • Restricts the implementation of taxonomy term meta via options. (Is that a thing?)

[adamsilverstein]

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?

[adamsilverstein]

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?

[swissspidy]

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.

[felixarntz]

@swissspidy I see #467 was merged, which is great. Could you maybe update the issue description to include that issue/PR (and any other related ones in progress or merged, if there are) so that we can keep track of the progress better?

[swissspidy]

I think we can close this one for now as we created sub-issues for the things we wanted to implement.