Skip to content
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

QOL Improvements #63

Merged
merged 9 commits into from
Oct 14, 2024
Merged

QOL Improvements #63

merged 9 commits into from
Oct 14, 2024

Conversation

ingeniumed
Copy link
Contributor

@ingeniumed ingeniumed commented Oct 11, 2024

Description

This builds on #59 by making some QOL improvements.

  • Remove the caching entirely from custom status and editorial metadata. This is something that's going to cause us more problems than it's worth. Instead, we shall now rely on WordPress's default caching strategy as well as VIP's various caching layers.
  • Switch our bespoke way of showing incompatibility PHP/WP notice to the new 6.4 way.
  • Switch our bespoke delete dialog for the built in one. I tried to move it to it's own function but honestly it didn't really gain us anything.
    Screenshot 2024-10-11 at 1 36 36 pm
  • A new logging utility that takes advantage of log2logstash within mu-plugins to send logs straight to logstash. This will only work on a VIP site which is what we are building for. This is only used in the notifications module right now, as we want users to know when emails or webhook notifications cannot be sent.
  • Remove the EM/CS mapping function for the admin script in favour of handling it on the client side. I've left the block editor one as is for now.
  • Add status codes to all our WP_Errors so they don't just give back 500

Steps to Test

  • Test drive everything, it should all work as expected.

@ingeniumed ingeniumed requested a review from a team as a code owner October 11, 2024 03:37
@ingeniumed ingeniumed self-assigned this Oct 11, 2024
@alecgeatches
Copy link
Contributor

@ingeniumed I reviewed your changes (and the deletion dialog changes) and they worked great!

I also pushed up an optimization change in 0ee148e. I was curious now that get_custom_statuses() wasn't cached how many times it was called, and how much of an effect caching had on the results. I added some basic instrumenting:

private static $get_custom_status_count    = 0;
private static $get_custom_status_duration = 0;

public static function get_custom_statuses(): array {
    if ( HelperUtilities::is_current_post_type_unsupported() ) {
        return self::get_core_statuses();
    }

    $start_time = microtime( true );

    // ... do get_custom_statuses() logic

    $end_time = microtime( true );
    $duration = $end_time - $start_time;

    self::$get_custom_status_duration += $duration;
    ++self::$get_custom_status_count;
    error_log( sprintf( 'get_custom_status() called %d times (%0.4fs) ', self::$get_custom_status_count, self::$get_custom_status_duration ) );

    return $statuses;
}

I also generated ~1000 posts with a variety of statuses to test:

Screenshot 2024-10-14 at 11 32 18 AM

Results

On the Admin -> Posts -> All Posts page (pictured above), get_custom_status() was called 34 times per request:

php-1 | NOTICE: PHP message: get_custom_status() called 1 times (0.0015s)
php-1 | NOTICE: PHP message: get_custom_status() called 2 times (0.0027s)
php-1 | NOTICE: PHP message: get_custom_status() called 3 times (0.0038s)
php-1 | NOTICE: PHP message: get_custom_status() called 4 times (0.0048s)
php-1 | NOTICE: PHP message: get_custom_status() called 5 times (0.0058s)
php-1 | NOTICE: PHP message: get_custom_status() called 6 times (0.0069s)
php-1 | NOTICE: PHP message: get_custom_status() called 7 times (0.0080s)
php-1 | NOTICE: PHP message: get_custom_status() called 8 times (0.0096s)
php-1 | NOTICE: PHP message: get_custom_status() called 9 times (0.0108s)
php-1 | NOTICE: PHP message: get_custom_status() called 10 times (0.0119s)
php-1 | NOTICE: PHP message: get_custom_status() called 11 times (0.0131s)
php-1 | NOTICE: PHP message: get_custom_status() called 12 times (0.0144s)
php-1 | NOTICE: PHP message: get_custom_status() called 13 times (0.0155s)
php-1 | NOTICE: PHP message: get_custom_status() called 14 times (0.0167s)
php-1 | NOTICE: PHP message: get_custom_status() called 15 times (0.0179s)
php-1 | NOTICE: PHP message: get_custom_status() called 16 times (0.0190s)
php-1 | NOTICE: PHP message: get_custom_status() called 17 times (0.0204s)
php-1 | NOTICE: PHP message: get_custom_status() called 18 times (0.0216s)
php-1 | NOTICE: PHP message: get_custom_status() called 19 times (0.0228s)
php-1 | NOTICE: PHP message: get_custom_status() called 20 times (0.0240s)
php-1 | NOTICE: PHP message: get_custom_status() called 21 times (0.0251s)
php-1 | NOTICE: PHP message: get_custom_status() called 22 times (0.0262s)
php-1 | NOTICE: PHP message: get_custom_status() called 23 times (0.0274s)
php-1 | NOTICE: PHP message: get_custom_status() called 24 times (0.0285s)
php-1 | NOTICE: PHP message: get_custom_status() called 25 times (0.0296s)
php-1 | NOTICE: PHP message: get_custom_status() called 26 times (0.0307s)
php-1 | NOTICE: PHP message: get_custom_status() called 27 times (0.0319s)
php-1 | NOTICE: PHP message: get_custom_status() called 28 times (0.0331s)
php-1 | NOTICE: PHP message: get_custom_status() called 29 times (0.0342s)
php-1 | NOTICE: PHP message: get_custom_status() called 30 times (0.0354s)
php-1 | NOTICE: PHP message: get_custom_status() called 31 times (0.0364s)
php-1 | NOTICE: PHP message: get_custom_status() called 32 times (0.0387s)
php-1 | NOTICE: PHP message: get_custom_status() called 33 times (0.0438s)
php-1 | NOTICE: PHP message: get_custom_status() called 34 times (0.0450s)

Curiously, checking the time for the first load (0.0015s) and multiplying it by 34, we get 0.051s, which is only a little bit slower than the actual time to gather statuses (0.045s). This means internal site caching isn't doing a whole lot to improve the speed of subsequent calls. However, ~40 ms isn't that bad.

I spent a bit debugging to see if there were any obvious functions that were responsible for the huge number of calls, and found that absolutely there was. core-hacks.php's fix_post_row_actions and fix_preview_link_part_one are each called on every single row of visible posts on the All Posts page. In 0ee148e I tried adding selective caching to these (no global cache, just a static used by each function separately). After that small fix, the new results were:

php-1 | NOTICE: PHP message: get_custom_status() called 1 times (0.0017s) 
php-1 | NOTICE: PHP message: get_custom_status() called 2 times (0.0030s) 
php-1 | NOTICE: PHP message: get_custom_status() called 3 times (0.0041s) 
php-1 | NOTICE: PHP message: get_custom_status() called 4 times (0.0054s) 
php-1 | NOTICE: PHP message: get_custom_status() called 5 times (0.0066s) 
php-1 | NOTICE: PHP message: get_custom_status() called 6 times (0.0077s) 
php-1 | NOTICE: PHP message: get_custom_status() called 7 times (0.0089s) 
php-1 | NOTICE: PHP message: get_custom_status() called 8 times (0.0100s)

As a result of this small optimization, we save about 4x the number of calls to get_custom_status() while keeping optimization code super localized to a function that benefits from it. I think this is much better than the old global cache strategy as it stays localized in "hack" functions, and reduces unnecessary calls by a lot, and doesn't cost much complexity.

I also tested get_custom_status() calls on the Workflow edit page, and there were only 3 there, so nothing worth optimizing.

I'm going to merge this PR for now, but let me know if you have any disagreements with the optimization or implementation. Thanks!

@alecgeatches alecgeatches merged commit 16d6ea1 into trunk Oct 14, 2024
5 checks passed
@alecgeatches alecgeatches deleted the refactor/part-2 branch October 14, 2024 17:50
@ingeniumed ingeniumed mentioned this pull request Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants