-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Refactor multisite and network admin checks for plugin screen #10146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor multisite and network admin checks for plugin screen #10146
Conversation
$is_multisite = is_multisite(); | ||
$is_network_admin = is_network_admin(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like these functions don't really do any work. They're either just looking at constants or the value of a class member variable. So doubt there would be any measurable improvement to doing this, but I'm happy to be wrong!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do the benchmark for function call and local variable use over 100 iterations and found that it will improve ~50%
time.
Average time spent for 1 x 100 iterations:
Use function - 5583
Store in local var - 1958
Change: -64.9%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the unit of time here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benchmark script
// Bootstrapping.
define('WP_USE_THEMES', true);
/** Loads the WordPress Environment and Template */
require ('wp-load.php');
// Setup.
$repetitions = 1;
$iterations = 100;
// Profiling.
for ( $repetition = 0; $repetition <= $repetitions; $repetition ++ ) {
if( $repetition == 0 ) {
continue;
}
$time_start = hrtime( true );
for ( $index = 0; $index < $iterations; $index ++ ) {
is_multisite();
is_network_admin();
}
$time_end = hrtime( true );
$function[ $repetition ] = $time_end - $time_start;
$time_start = hrtime( true );
$is_multisite = is_multisite();
$is_network_admin = is_network_admin();
for ( $index = 0; $index < $iterations; $index ++ ) {
$call_m = $is_multisite;
$call_n = $is_network_admin;
}
$time_end = hrtime( true );
$local_var[ $repetition ] = $time_end - $time_start;
}
$function_avg = array_sum( $function ) / $repetitions;
$local_var_avg = array_sum( $local_var ) / $repetitions;
$percentage = round( $local_var_avg / $function_avg * 100, 1 ) - 100;
printf( "Average time spent for {$repetitions} x {$iterations} iterations:<br>" );
printf( "Use function - {$function_avg} <br>");
printf( "Store in local var - {$local_var_avg}<br>" );
printf( "Change: {$percentage}%%" );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so hrtime()
returns nanoseconds. That means 5,583 nanoseconds is 0.005583 milliseconds. This is extremely tiny. I don't think it's necessarily bad to make this change, except for possibility that a plugin could override these globals and cause something unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @westonruter! What do you think should be the next step here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't really see the need for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Close the Ticket and PR.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Trac ticket: https://core.trac.wordpress.org/ticket/64077
This PR refactors the
src/wp-admin/plugins.php
file to improve efficiency and maintainability by caching the results ofis_multisite()
andis_network_admin()
in local variables. All subsequent checks throughout the file now use these cached variables, reducing repeated function calls and improving readability. No functional behavior is changed.Refactoring for efficiency and readability:
is_multisite()
andis_network_admin()
results in$is_multisite
and$is_network_admin
variables at the top of the file, and replaced all repeated calls to these functions with the variables throughout the plugin management logic.is_multisite()
andis_network_admin()
to use the new local variables, including plugin activation, deactivation, automatic updates, and UI display logic.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.