github-actions[bot]
commented
1 month ago Size Change: 0 B
Total Size: 2.77 MB
Closed Swanand01 closed 1 month ago
github-actions[bot]
commented
1 month ago Size Change: 0 B
Total Size: 2.77 MB
googleforcreators-bot
commented
1 month ago Plugin builds for 1080062d68722b80b2894dcfc6646ada2c6bc9bd are ready :bellhop_bell:!
swissspidy
commented
1 month ago While reviewing this I noticed a few other places where we could optimize our database queries. Granted, this PR here makes such optimizations a bit redundant, as now they will all be primed.
Thinking out loud, here's what I found:
This call here causes a database query on every page load:
Removing this call would require all sorts of changes to other classes depending on this default value. So probably not worth it.
This call also causes a db query on every page load:
This is not something that the priming can address, as it's not our own option we're loading here.
We can probably refactor to something like this:
add_filter(
'googlesitekit_analytics-4_tag_amp_blocked',
function ( $blocked ) {
$handler = $this->settings->get_setting( $this->settings::SETTING_NAME_TRACKING_HANDLER );
if ( 'web-stories' === $handler && $this->context->is_web_story() ) {
return true;
}
return $blocked;
}
);
add_action(
'web_stories_print_analytics',
function () {
$handler = $this->settings->get_setting( $this->settings::SETTING_NAME_TRACKING_HANDLER );
if ( 'site-kit' === $handler && $this->is_analytics_module_active() ) {
remove_action( 'web_stories_print_analytics', [ $this->analytics, 'print_analytics_tag' ] );
}
},
5
);Another database query caused by this on every admin page:
which calls get_setting and get_plugin_status() methods, which call the db.
Potential solution: maybe split up get_dashboard_settings() method into two or so.
On my local site I also noticed that a lot of options are autoloaded that probably shouldn't.
Some countermeasures to consider:
wp_set_option_autoload_values in a new database migration\Google\Web_Stories\Settings::update_setting() to support a third optional $autoload parameter. Then, explicitly pass this in places where we update options. Not sure I like that one though.swissspidy
commented
1 month ago OK that was quite the rabbit hole I went down here 😅
I found quite a few services that were being registered too early, causing unnecessary database queries. Also, some queries happened even before the priming happened.
The web_stories_customizer_settings is now also registered so that it belongs to the same settings group that is primed at the beginning.
Now there are indeed only the 2 expected database queries from the priming.
Summary
This PR adds the use of
wp_prime_option_caches_by_groupto combine queries forweb-storiesrelated options on the dashboard.User-facing changes
None.
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
No.
Does this PR change what data or activity we track or use?
No.
Does this PR have a legal-related impact?
No.
Checklist
Type: XYZlabel to the PRFixes #13523