Make WordPress Core

#58069 closed defect (bug) (fixed)

Performance of _wp_normalize_relative_css_links() can be increased >2x

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 6.2.1 Priority: normal
Severity: normal Version: 5.9
Component: Script Loader Keywords: has-unit-tests fixed-major has-patch
Focuses: css, performance Cc:

Description (last modified by westonruter)

The _wp_normalize_relative_css_links() function is not currently implemented in an optimal way. It is doing a regex match for all instances of url(...) in a given CSS, and then for each match it checks if it is a relative URL. If so, it then makes the URL absolute and then does another global search & replace in the CSS to replace the relative path with the absolute path. This means for every search there is a secondary search. This secondary str_replace() can be eliminated by replacing the unitial preg_match_all() with a preg_replace_callback(). This makes the function run >2x faster when benchmarked being run across each CSS file in Gutenberg and the built-in themes.

$ composer run-script test -- --iterations=10 --revs=500
> phpbench run Bench.php --report=default '--iterations=10' '--revs=500'
PHPBench (1.2.10) running benchmarks... #standwithukraine
with PHP version 7.4.33, xdebug , opcache 

\Bench

    benchTrunk..............................I9 - Mo7.734ms (±2.96%)
    benchPatch..............................I9 - Mo3.527ms (±6.74%)

Subjects: 2, Assertions: 0, Failures: 0, Errors: 0
+------+-----------+------------+-----+------+------------+-------------+--------------+----------------+
| iter | benchmark | subject    | set | revs | mem_peak   | time_avg    | comp_z_value | comp_deviation |
+------+-----------+------------+-----+------+------------+-------------+--------------+----------------+
| 0    | Bench     | benchTrunk |     | 500  | 4,242,960b | 8,066.348μs | +0.76σ       | +2.24%         |
| 1    | Bench     | benchTrunk |     | 500  | 4,242,960b | 8,006.150μs | +0.50σ       | +1.48%         |
| 2    | Bench     | benchTrunk |     | 500  | 4,242,960b | 7,815.732μs | -0.32σ       | -0.93%         |
| 3    | Bench     | benchTrunk |     | 500  | 4,242,960b | 7,706.406μs | -0.78σ       | -2.32%         |
| 4    | Bench     | benchTrunk |     | 500  | 4,242,960b | 7,702.188μs | -0.80σ       | -2.37%         |
| 5    | Bench     | benchTrunk |     | 500  | 4,242,960b | 7,649.464μs | -1.03σ       | -3.04%         |
| 6    | Bench     | benchTrunk |     | 500  | 4,242,960b | 7,628.240μs | -1.12σ       | -3.31%         |
| 7    | Bench     | benchTrunk |     | 500  | 4,242,960b | 7,785.394μs | -0.45σ       | -1.32%         |
| 8    | Bench     | benchTrunk |     | 500  | 4,242,960b | 8,314.294μs | +1.82σ       | +5.39%         |
| 9    | Bench     | benchTrunk |     | 500  | 4,242,960b | 8,219.126μs | +1.41σ       | +4.18%         |
| 0    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,492.050μs | -0.80σ       | -5.42%         |
| 1    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,440.668μs | -1.01σ       | -6.81%         |
| 2    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,524.826μs | -0.67σ       | -4.53%         |
| 3    | Bench     | benchPatch |     | 500  | 4,242,960b | 4,035.964μs | +1.38σ       | +9.32%         |
| 4    | Bench     | benchPatch |     | 500  | 4,242,960b | 4,178.894μs | +1.96σ       | +13.19%        |
| 5    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,769.186μs | +0.31σ       | +2.09%         |
| 6    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,527.606μs | -0.66σ       | -4.45%         |
| 7    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,900.464μs | +0.84σ       | +5.65%         |
| 8    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,542.204μs | -0.60σ       | -4.06%         |
| 9    | Bench     | benchPatch |     | 500  | 4,242,960b | 3,508.126μs | -0.74σ       | -4.98%         |
+------+-----------+------------+-----+------+------------+-------------+--------------+----------------+

Change History (15)

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


13 months ago
#1

  • Keywords has-patch has-unit-tests added
  • Replace preg_match_all() and its secondary str_replace() call with preg_replace_callback().
  • Fix case where paths beginning with http and https (but not http: and https:) were erroneously not counted as relative.
  • Improve code style and readability by consolidating conditions and returning once.

Trac ticket: https://core.trac.wordpress.org/ticket/58069

#2 @westonruter
13 months ago

  • Component changed from Editor to Script Loader

#3 @westonruter
13 months ago

  • Description modified (diff)

#4 @westonruter
13 months ago

  • Description modified (diff)

#5 @adamsilverstein
13 months ago

Nice work @westonruter - PR look good to me, thanks for providing the performance benchmarks as well.

#6 @westonruter
12 months ago

Great. I plan to commit today or tomorrow.

#7 @westonruter
12 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 55658:

Script Loader: Optimize performance of _wp_normalize_relative_css_links() by more than 2x.

  • Replace preg_match_all() and its secondary str_replace() call with preg_replace_callback().
  • Fix case where paths beginning with http and https (but not http: and https:) were erroneously not counted as relative.
  • Improve code style and readability by consolidating conditions and returning once.
  • Use str_starts_with() consistently instead of strpos().

Follow-up to [52036], [52695], and [52754].

Fixes #58069.
See #54243.

#9 @westonruter
12 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 6.2.1 consideration.

#10 @azaozz
12 months ago

  • Keywords changes-requested added; has-patch removed

Sorry, a nitpick :)

The commit message says "Improve code style and readability" but I tend to disagree. Both style and readability seem worse after the commit.

Negating a long multi-line list of function calls wrapped in braces doesn't help readability. Better readability can be achieved by a short-circuit imho. I.e. something like (untested):

if (
   str_starts_with( $url, 'http:' ) ||
   str_starts_with( $url, 'https:' ) ||
   str_starts_with( $url, '//' ) ||
   ....
) {
   return $prefix . $url;
}

$absolute_url = ...
// etc.

Using (yet another) style for multi-line conditionals (every || on its own line) doesn't help it either. I know WPCS doesn't flag this as "error" but why should the style here be different than the rest of the WP code?

Last edited 12 months ago by azaozz (previous) (diff)

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


12 months ago
#11

  • Keywords has-patch added

#12 @azaozz
12 months ago

In [55669]:

Script Loader: Improve code style and readability in _wp_normalize_relative_css_links().

Props: westonruter.
See: 58069.

#13 @azaozz
12 months ago

  • Keywords changes-requested removed

#15 @audrasjb
12 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 55736:

Script Loader: Optimize performance of _wp_normalize_relative_css_links() by more than 2x.

  • Replace preg_match_all() and its secondary str_replace() call with preg_replace_callback().
  • Fix case where paths beginning with http and https (but not http: and https:) were erroneously not counted as relative.
  • Improve code style and readability by consolidating conditions and returning once.
  • Use str_starts_with() consistently instead of strpos().

Follow-up to [52036], [52695], and [52754].

Props westonruter, adamsilverstein, azaozz.
Merges [55658] and [55669] to the 6.2 branch.
Fixes #58069.
See #54243.

Note: See TracTickets for help on using tickets.