Make WordPress Core

Opened 2 years ago

Closed 10 months ago

Last modified 6 months ago

#55270 closed defect (bug) (fixed)

Option can_compress_scripts not autoload causing extra DB query on every request

Reported by: ravanh's profile RavanH Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: minor Version:
Component: Options, Meta APIs Keywords: dev-feedback
Focuses: performance Cc:

Description

Asked about this two years ago https://wordpress.org/support/topic/option-can_compress_scripts-not-autoload/ but noticed this is still the case:

The option can_compress_scripts not being among the autoload options creates an extra DB query on every (front-end and admin) request:

SELECT option_value FROM wp_options WHERE option_name = 'can_compress_scripts' LIMIT 1

Install Debug Bar and you'll notice it everywhere.

WordPress can very easily prevent this extra query (for one option!) by making it an autoload option by putting it into the array of $defaults in populate_options() in wp-admin/includes/schema.php and removing it from the $unusedoptions array...

Next, an upgrade routine to make it autoload for current sites on upgrade... And maybe rethink the Network approach: currently stored as a network option, but is it really a network option? It gets updated on every odd sub-site admin (not super-admin!) request via compression_test() in admin-footer.php anyway so why not make it a normal blog option?

Note that it has been in the $unusedoptions array since WordPress 1.5 but re-introduced as an actively used DB option in 2.8 in script-loader.php and admin-footer.php.

Change History (27)

This ticket was mentioned in PR #2365 on WordPress/wordpress-develop by RavanH.


2 years ago
#1

  • Keywords has-patch added

The option can_compress_scripts not being among the autoload options creates an extra DB query on every (front-end and admin) request:

`
SELECT option_value FROM wp_options WHERE option_name = 'can_compress_scripts' LIMIT 1
`

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

#2 @costdev
2 years ago

  • Version trunk deleted

Removing trunk from Version as this has existed before the 6.0 cycle.

#3 @spacedmonkey
12 months ago

#57836 was marked as a duplicate.

#4 @spacedmonkey
12 months ago

  • Focuses performance added

#5 @azaozz
12 months ago

Actually thinking this functionality (the option and the code that sets it) can now be removed. Detecting whether compression from PHP is supported was necessary many years ago and was used when outputting concatenated and compressed JS and CSS from load-scripts.php and load-styles.php. As PHP based compression was removed there, the code that detects it is unused.

Also looked if the option is used in themes/plugins. It is mentioned in about 40 plugins but seems only used as a "default core option", not to determine if compression from PHP is available. That's not surprising as over the years output compression has moved to the server software as it is more efficient.

In these terms thinking it would be better to (slightly) repurpose this ticket and remove the code that sets the option too. Seems for back-compat with the ~40 plugins the option can still be pre-populated with false.

Last edited 12 months ago by azaozz (previous) (diff)

#7 @spacedmonkey
12 months ago

  • Milestone changed from Awaiting Review to 6.3
  • Owner set to spacedmonkey
  • Status changed from new to assigned

#8 follow-up: @spacedmonkey
12 months ago

@azaozz I think that is another for ticket that. This is a simple fix that can go out in the next release.

I have added an new PR at #4447.

#9 in reply to: ↑ 8 ; follow-up: @azaozz
12 months ago

Replying to spacedmonkey:

@azaozz I think that is another for ticket that. This is a simple fix that can go out in the next release.

Sure, it makes sense to switch the option to not autoloading and close this ticket. I'll open another ticket for removing the rest of that code.

Not so sure about the rest of the changes though (the update_option() vs. update_site_option()). They look like an enhancement of a unused feature that is going to be removed soon?

Last edited 12 months ago by azaozz (previous) (diff)

#10 @azaozz
12 months ago

Related: #58302.

#11 in reply to: ↑ 9 @spacedmonkey
11 months ago

Replying to azaozz:

Not so sure about the rest of the changes though (the update_option() vs. update_site_option()). They look like an enhancement of a unused feature that is going to be removed soon?

But it is currently used right? It is used here. Until #58302 is committed which maybe in a future release ( if ever ), then we should fix what have.

#12 @flixos90
11 months ago

@azaozz I see your point, it seems pointless to autoload an option which will be removed soon. However, I also agree with @spacedmonkey that it is worth doing so to improve performance for the time being. If we get to a solution for #58302 that is committed in time for 6.3, then this change will naturally become pointless. But if the other ticket is more complicated than we think and ends up delayed, then it's reasonable to improve performance here.

In other words, I think this is worth moving forward regardless of #58302 - when the latter is implemented, it'll remove these parts of the code anyway.

That said, @spacedmonkey it would be great if you could share some performance benchmark to see what impact this has.

#13 @spacedmonkey
11 months ago

@flixos90
500 runs, on 2013 theme

Trunk PR
Response Time (median) 111.59 111.3
wp-load-alloptions-query (median) 0.63 0.64
wp-before-template (median) 24.2 23.9
wp-before-template-db-queries (median) 3.26 3.24
wp-template (median) 78.62 78.59
wp-total (median) 103.76 102.76
wp-template-db-queries (median) 16.75 16.78

Not a massive server response time improvement, but it is an improvement to database performance, as it removes a database query for EVERY page request, even REST API request.

#14 @spacedmonkey
11 months ago

  • Component changed from Script Loader to Options, Meta APIs

#15 @spacedmonkey
11 months ago

  • Keywords commit added

#16 @spacedmonkey
11 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55854:

Options, Meta APIs: Change the option can_compress_scripts to be autoloaded.

Ensure that the option can_compress_scripts is autoloaded on single sites, as this option is used in all requests. This change saves one database query per page request.

Props RavanH, spacedmonkey, costdev, azaozz, flixos90.
Fixes #55270.

#18 @azaozz
11 months ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Not sure what's going on here but the patch doesn't seem to be doing what the ticket description and the commit message is saying?

  1. The problem this ticket it trying to solve is that on older sites the can_compress_scripts option was added with autoload = 'no'. This patch was supposed to delete the old option and replace it with a new one with autoload = 'yes' which is the default. That should happen for both single site and multisite, however that is not the case? In addition there seem to be plugins that check this option so it has to always be set.
  2. The conditional that runs either update_option() or update_site_option() doesn't seem to be doing anything as that code runs "once in a blue moon". I.e. this part of the patch is totally pointless.
  3. The 'yes' in update_option( 'can_compress_scripts', 0, 'yes' ); is redundant and doesn't do anything. It is the default and also the option is switched to autoloading in function upgrade_630() (at least on single sites).
Last edited 11 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


11 months ago

#20 @spacedmonkey
11 months ago

Before this change, update_site_option was called even on single site. If the option does exist, like on the first run, it falls back to add_network_option and then $result = add_option( $option, $value, '', 'no' );. This is why the change was made to use update_option( 'can_compress_scripts', 0, 'yes' ), on the first run of this code, update_option falls back to add_option. Adding autoload = yes, might be overkill, it is helpful, as it makes the point of this code much clearer.

The commit works as expected in my testing. Can you give me steps to replicate a bug and I can see if I work on a fix.

#21 follow-up: @spacedmonkey
10 months ago

Awaiting feedback from @azaozz, otherwise I will close.

#22 in reply to: ↑ 21 @azaozz
10 months ago

Replying to spacedmonkey:

Awaiting feedback from @azaozz, otherwise I will close.

Uhh, I wanted to investigate the reason this code bypasses the fallback in update_site_option() to update_option() on single sites, and eventually either fix similar cases (think there are few more in core) or open a ticket to fix the fallback behavior.

Unfortunately haven't had any time for that yet. Lets keep this open for a bit longer so it doesn't "fall off the radar".

#23 follow-up: @spacedmonkey
10 months ago

Hello @azaozz

Have you had a chance to re-review this ticket. In my testing, I am haven't seen any issues.
Can take a look at this. If you don't have time, can you close the ticket out and we can get back to it at a future date. Thanks.

#24 @spacedmonkey
10 months ago

  • Keywords dev-feedback added; needs-patch removed

#25 in reply to: ↑ 23 @azaozz
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to spacedmonkey:

Sure, it's fixed, lets close it.

This ticket was mentioned in Slack in #core by scottbuscemi. View the logs.


9 months ago

#27 @flixos90
6 months ago

#56912 was marked as a duplicate.

Note: See TracTickets for help on using tickets.