Make WordPress Core

Opened 7 weeks ago

Closed 10 days ago

#61782 closed defect (bug) (maybelater)

Memoize wp_get_wp_version

Reported by: cybr's profile Cybr Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Priority: normal
Severity: normal Version: trunk
Component: General Keywords: has-patch dev-feedback close
Focuses: Cc:

Description

wp_get_wp_version() currently requires a file whenever it's being called. This file is being required every time the function is called.

<?php
function wp_get_wp_version() {
        require ABSPATH . WPINC . '/version.php';

        return $wp_version;
}

We can memoize the return value using the static keyword. This will make the file-requiring happen only once. At just 1000 iterations, this memoization is already 160 times faster (from 0.1000s to 0.0006s execution time).

<?php
function wp_get_wp_version() {

        static $wp_version;

        if ( empty( $wp_version ) )
                require ABSPATH . WPINC . '/version.php';

        return $wp_version;
}

Change History (15)

#1 @Cybr
7 weeks ago

Here's a plugin to test my claim: https://gist.github.com/sybrew/bd78076fabdd6ce67ce25e57a8fc7e72.

Just upload the PHP file as a plugin, activate it, and visit any public page with the "?test=1" query parameter.

Correction: I tested 10,000 iterations, not 1000 iterations. But testing 1000 will still yield the same ~160x performance improvement. At 10 iterations, we measure a ~60x performance improvement, and at 2 iterations ~2x.

Last edited 7 weeks ago by Cybr (previous) (diff)

This ticket was mentioned in PR #7105 on WordPress/wordpress-develop by @debarghyabanerjee.


7 weeks ago
#2

  • Keywords has-patch added

Trac Ticket: #61782

## Problem Statement:

  • The wp_get_wp_version() function currently requires the version.php file on every call. This results in unnecessary file inclusion, leading to inefficiencies especially when the function is invoked multiple times. Performance testing indicates that the execution time can be reduced dramatically with this change—memoization shows a performance improvement of approximately 160 times (from 0.1000s to 0.0006s) after 1000 iterations.

## Solution

  • Optimization: Utilize the static keyword to cache the WordPress version within the wp_get_wp_version() function. This change ensures that the version.php file is required only once, and subsequent calls will return the cached version number without additional file operations.
  • Performance: This modification results in a significant reduction in execution time by avoiding redundant file inclusions, leading to more efficient code execution and improved overall performance.

## Benefits

  • Performance Improvement: Reduces the overhead of including the version.php file on each function call, resulting in faster execution times.
  • Resource Efficiency: Decreases the number of file I/O operations, which is beneficial for high-traffic sites or applications that call wp_get_wp_version() frequently.
  • Cleaner Code: Simplifies the function logic by leveraging caching to handle repeated requests more efficiently.

This ticket was mentioned in PR #7105 on WordPress/wordpress-develop by @debarghyabanerjee.


7 weeks ago
#3

Trac Ticket: #61782

## Problem Statement:

  • The wp_get_wp_version() function currently requires the version.php file on every call. This results in unnecessary file inclusion, leading to inefficiencies especially when the function is invoked multiple times. Performance testing indicates that the execution time can be reduced dramatically with this change—memoization shows a performance improvement of approximately 160 times (from 0.1000s to 0.0006s) after 1000 iterations.

## Solution

  • Optimization: Utilize the static keyword to cache the WordPress version within the wp_get_wp_version() function. This change ensures that the version.php file is required only once, and subsequent calls will return the cached version number without additional file operations.
  • Performance: This modification results in a significant reduction in execution time by avoiding redundant file inclusions, leading to more efficient code execution and improved overall performance.

## Benefits

  • Performance Improvement: Reduces the overhead of including the version.php file on each function call, resulting in faster execution times.
  • Resource Efficiency: Decreases the number of file I/O operations, which is beneficial for high-traffic sites or applications that call wp_get_wp_version() frequently.
  • Cleaner Code: Simplifies the function logic by leveraging caching to handle repeated requests more efficiently.

#4 @SergeyBiryukov
7 weeks ago

  • Milestone changed from Awaiting Review to 6.7

#5 @SergeyBiryukov
7 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 58827:

General: Memoize the return value in wp_get_wp_version().

This aims to optimize performance by saving the return value to a static variable, so that the version.php file is not unnecessarily required on each function call.

Follow-up to [58813].

Props Cybr, debarghyabanerjee, mukesh27.
Fixes #61782. See #61627.

@SergeyBiryukov commented on PR #7105:


7 weeks ago
#6

Thanks for the PR! Merged in r58827.

#7 @costdev
7 weeks ago

  • Keywords dev-feedback added

@SergeyBiryukov This new helper function may be used during the upgrade routines (or by plugins with post-upgrade hooks, for example), where wp-includes/version.php will have been replaced, and OPcache will have been invalidated. However, the return value of wp_get_wp_version() won't match the actual current version until the next request. This may need some further thought and testing.

A $force parameter may have been useful to include here, to force a fresh file load in such circumstances.

Last edited 7 weeks ago by costdev (previous) (diff)

#8 @SergeyBiryukov
7 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to costdev:

This new helper function may be used during the upgrade routines (or by plugins with post-upgrade hooks, for example), where wp-includes/version.php will have been replaced, and OPcache will have been invalidated. However, the return value of wp_get_wp_version() won't match the actual current version until the next request. This may need some further thought and testing.

Good catch, thanks!

#9 @peterwilsoncc
6 weeks ago

My inclination is to revert [58827].

While 10,000 iterations shows a drop from 0.1000s to 0.0006s in execution time, it's more likely the function will only be called a few times in the typical execution of a WordPress request.

Given the risk of the incorrect version being reported pre- and post- upgrade a more real-world example of the improvement is needed.

#10 @Cybr
6 weeks ago

I concur with Peter. You can skip to the last paragraph to get the conclusion.

Still, a single parameter to force fetching the latest version could rewrite the cache (lest we not forget to use it):

<?php
function wp_get_wp_version( $reset = false ) {

        static $wp_version;

        if ( ! isset( $wp_version ) || false !== $reset )
                require ABSPATH . WPINC . '/version.php';

        return $wp_version;
}

I used false !== $reset here so we can write wp_get_version( 'latest' ) or something, but that's a coding style preference. true is not conveying much as a parameter, but that's probably best left for another discussion.

On WP 6.6.1 and WP 6.7-alpha-58846, on PHP 8.2 with OPcache, using Twenty Twenty-Two, no plugins, logged in, on the homepage, version.php is included 3 times:

  • 1x wp_default_styles(),
  • 1x wp-settings.php,
  • 1x wp_scripts_get_suffix().

In that context, with just 3 plausible calls to wp_get_wp_version(), the function, as per this comment, would save 0.04ms of load time compared to the non-memoizing function.

At /wp-admin/index.php on 6.7-alpha-58846, wp_get_wp_version() is called 2 times:

  • 1x _maybe_update_core(),
  • 1x core_update_footer().

The version.php file is included 4 times:

  • 1x wp-settings.php,
  • 1x wp_scripts_get_suffix(),
  • 1x wp_default_styles(),
  • 1x wp_get_wp_version().

In that context, with just 2 plausible calls to wp_get_wp_version(), the function would save 0.03ms.

The total load time of WordPress on the server I tested this with is about 60ms.

So, this can save anywhere from 0.05% to 0.06% of load time (one-1500th to one-2000th of a relative impact).

I'm pedantic and overly ambitious. Ignoring the law of diminishing returns and Zeno's paradox, with 1500 iterations of this kind of improvement, WP would load instantly.

Realistically, however, 0.04ms of total savings isn't a lot.

Last edited 6 weeks ago by Cybr (previous) (diff)

#11 @peterwilsoncc
6 weeks ago

In 58848:

General: Removing static from wp_get_wp_version().

Removes the static storing the version number in wp_get_wp_version() to ensure the version number is reported correctly after a WordPress upgrade is completed.

Reverts [58827].

Props costdev, SergeyBiryukov, Cybr.
See #61782.

#12 @peterwilsoncc
6 weeks ago

Thanks for looking in the stats, @Cybr.

I've reverted the commit for now but am leaving the ticket open to consider performance improvements in the future as the function gains wider use within Core.

I considered using a non-persistent cache to store the value and flushing it during the upgrade but that failed the though experiment of managing plugins modifying the value: the security through obscurity advise would simply recommend changing both the global and the cached value.

Either a $force parameter or caching a boolean to determine if the value has been populated may be possible.

#13 @TobiasBg
6 weeks ago

I don't think that a $force parameter makes much sense. As a developer, when calling this function, I would always want the correct version, so I would be setting the $force parameter. But if everybody uses that parameter anyways, there's not much benefit to even offering it. The $force outcome should then be the default (as it is now after the revert) and would save the overhead of additional variable checks.

#14 @desrosj
2 weeks ago

  • Keywords close added

I tend to agree that a $force parameter feels a bit out of place. Plus, if the default value is false, then anything expecting an accurate version to be returned may break when 6.7 is released.

I think this is just one of those improvements that changes old, long standing code that's just not worth the risk. I'm adding a close suggestion based on the feedback from @TobiasBg and I.

#15 @peterwilsoncc
10 days ago

  • Milestone 6.7 deleted
  • Resolution set to maybelater
  • Status changed from reopened to closed

I agree with @desrosj & @TobiasBg that using a static and a $force parameter isn't worth the benefit a static or other caching provides.

As the use of the function increases, it may be worth reconsidering so I've closed this as maybelater.

Note: See TracTickets for help on using tickets.