Performance issue on user_login() method hooked on “wp_login”

  • Resolved mtxz

    (@mtxz)


    1 year ago

    Hello,

    Im using WP 6.4.1 and BNFW 1.9.3, Debian Nginx PHP 8.1, custom Sage 9 theme. This is a Woocommerce store with thousands of products. So I have thousands of posts, and millions of post_meta. Also note that I’m using WP Index for MySQL.

    I found a performance issue using your plugin. The issue is related to the user_login() hooked on the wp_login action (bnfw.php line 200 and bnfw.php line 1080).

    I’m using a custom authentication flow that calls do_action('wp_login'...) at some point. Your plugin has a method hooked on this action. BNFW calls $this->notifier->get_notifications( 'user-login' ) that trigger’s a long SQL request on the database to find BNFW notifications. Note that I don’t have any “on-login” notification enabled, but I assume the plugin must check that anyway.

    I debugged the entire login process and wp_loginhooked methods, until I found your method, which was de culprit (among dozens of other hooked actions on wp_login.

    The SQL request produced takes 56 seconds to run on my server:

    Query needs to create temporary table to run:

    I was able to remove your plugin’s hooked method. But it certainly means “disabling” the “on-login bnfw notifications”

    add_action( 'plugins_loaded', 'bnfw_remove_login_plugin_filter' );
add_action( 'init', 'bnfw_remove_login_plugin_filter', 200 );
add_action( 'wp_login', function ( $user_login, $user ) {
    bnfw_remove_login_plugin_filter();

    return $user_login;
}, 1, 2 );

function bnfw_remove_login_plugin_filter() {
    if(class_exists( 'BNFW', false )) {
        $instance = \BNFW::factory();
        remove_action( 'wp_login', array( $instance, 'user_login' ), 10 );
        remove_action( 'wp_login', array( 'BNFW', 'user_login' ), 10 );
    }
}

    I believe I could also use an Object Cache (eg: WP Redis) plugin to cache this request. I also fear that other BNFW notification triggers could produce the same kind of queries.

    Debugging the SQL Query, it seems that the huge delays come from the GROUP BY wp_posts.ID. Removing the group_by makes the query less than 1s (from 60s to 1s removing the group_by).

    From more tests on the WP_Query args generated by your method, it all seems pretty “classical”, and the GROUP_BY is added by WP. Idk what i could do to optimise this.

    I believe that it’s related to multiple INNER JOINs on same table. Found this also this

    FIY

    Thank you.

    • This topic was modified 1 year ago by mtxz.
Viewing 7 replies - 1 through 7 (of 7 total)
  • Plugin Author bnfw

    (@voltronik)

    Hi @mtxz,
    Thank you so much for reporting this and putting in so much time and effort to detail everything clearly. I’ll look into this with a view to incorporate changes into the next update of the plugin.

    If I have any questions, I’ll come back to you here.

    Thanks again.

    Thread Starter mtxz

    (@mtxz)

    Hello,

    As expected, I encountered another of your hook causing delays. This time when doing wp_insert_term. I’ll need to remove this hook too, as it slows my product sync a lot…

    I really think it would improve to use raw SQL queries (wpdb), or two queries. One to get post_ids from meta, the other to get the posts.

    Thanks

    Plugin Author bnfw

    (@voltronik)

    Hi @mtxz,
    Thanks for this additional report. I have it on my roadmap to investigate these for inclusion in the next release, once tested.

    Thread Starter mtxz

    (@mtxz)

    Hello,

    any news about this? I still see a lot of slow query on user registration, as I’m using the notification for user registration.

    Would it be possible to implement a quick fix or to use transients that are purged upon BNFW admin update? I dont find any filter to do this.
    I could edit the query, or change the resulting posts, but I cannot prevent the query to be triggered.

    Thanks

    Thread Starter mtxz

    (@mtxz)

    I ended up making a fork of your plugin to add transient features on the get_notification() method.

    I just set a transient for single notification queries, saved the resulting post IDs, and changed the wp_query args. (“frontend” query only, so enabled ones only). And clear the transient upon notification post-save.

    Screenshot are truncated, but the transient key is generated using $types[0]

    For purge, I get the notification post meta (bnfw_notification) your plugin set, that contains the notification “slug identifier”.

    I also added in my them a hook on “post_updated” to check for bnfw_notification post_type and clear the transient again to be sure.

    • This reply was modified 11 months, 3 weeks ago by mtxz.
    • This reply was modified 11 months, 3 weeks ago by mtxz.
    Thread Starter mtxz

    (@mtxz)

    I updated the script to :

    • check if get_transient returns and empty array. If so, return an empty array from get_notification() (or system don’t cache the empty result, and continue to execute the query again DB)
    • Added $exclude_disabled to the transient key so I have cache for both queries. Edited transient purge accordingly.
    Plugin Author bnfw

    (@voltronik)

    Hi @mtxz,
    Thanks for this additional information.

    I’m still investigating this but won’t have any changes, if there are any, to add to a new release until next year. The next release will be a fairly large bug fix and performance release.

Viewing 7 replies - 1 through 7 (of 7 total)
  • The topic ‘Performance issue on user_login() method hooked on “wp_login”’ is closed to new replies.