Make WordPress Core

Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#59599 closed defect (bug) (fixed)

Possible performance regression: script strategy `defer` with dependent in footer

Reported by: adamsilverstein's profile adamsilverstein Owned by: westonruter's profile westonruter
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.3
Component: Script Loader Keywords: dev-feedback has-patch has-unit-tests commit
Focuses: performance Cc:

Description

I have been working on adopting the new Script API strategy feature to add defer to scripts that were previously placed in the footer and discovered a possible performance regression.

Here is the scenario:

  • I am working on a plugin "Plugin A" which currently enqueues a script on the front end, using in_footer => true, so the script is printed in the footer and so is mostly non blocking (it still blocks any resources loaded after it in the footer, so technically it is still blocking).
  • In WP >= 6.3 the plugin adopts the new strategy feature and changes in_footer to array( 'strategy => 'defer' ). The goal here is to make the script truly non-blocking, and move it to the header where it can be discovered earlier.

So far, so good. Except...

  • Along comes "Plugin B", it enqueues a front end script that depends on the script from "Plugin A", and it specifies in_footer => true (it is not marked for defer).
  • In this case the script from "Plugin A" can no longer be deferred (since the script from "Plugin B" depends on it and this would break execution order. So now the script from "Plugin A" is not deferred and it remains in the header.

Result: "Plugin A" now has a blocking script in the header - a worse situation then where we started.

Potential solution

Core already has some dependency handling of in_footer behavior built in that can move scripts to the header, maybe we can extend this.

Take this scenario:

  • Script A is marked for the footer - in_footer = > true.
  • Script B depends on Script A and is marked for the header - in_footer => false

In this case, core will place both scripts in the header group - ie. Script A is moved to the header to ensure that it executes before its dependent - Script B.

Maybe we can layer on this additional behavior:

  • Script A is marked for the header and defer is not deferrable because of a Script B dependency
  • Script B depends on script A and is in the footer

Core would then move both scripts to the footer group.
Result: Both scripts are output in the footer.

We could consider expanding this to any header scripts with footer dependencies (ie. not just limited to those that are mean to to be deferred). This would be a more risky change though since developers would be expecting execution in the header.

Attachments (3)

58599.diff (1.1 KB) - added by adamsilverstein 14 months ago.
58599.2.diff (4.9 KB) - added by adamsilverstein 14 months ago.
58599.3.diff (10.3 KB) - added by adamsilverstein 14 months ago.

Download all attachments as: .zip

Change History (24)

#1 @adamsilverstein
14 months ago

  • Focuses performance added

#2 @adamsilverstein
14 months ago

  • Milestone changed from Awaiting Review to 6.4

This ticket was mentioned in Slack in #core-performance by adamsilverstein. View the logs.


14 months ago

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


14 months ago
#4

  • Keywords has-patch added; needs-patch removed

#5 @adamsilverstein
14 months ago

Posted a potential fix in 58599.diff.

This could probably use some unit tests to confirm the behavior.

I tested manually by adding two dummy scripts and experimenting with their 'in_footer' settings:

<?php
// Register a dummy script that depends on jquery-blockui, forcing that script to be non-deferrable.
wp_enqueue_script(
        'script-a',
        plugin_dir_url( __FILE__ ) . 'assets/script-a.js',
        array( ),
        '1.0.0',
        array( 'strategy'  => 'defer' ) // Placing this script in the header.
);
wp_enqueue_script(
        'script-b',
        plugin_dir_url( __FILE__ ) . 'assets/script-b.js',
        array( 'script-a' ),
        '1.0.0',
        array( 'strategy'  => 'blocking', 'in_footer' => true ) // Placing this script in the footer.
);

for script b, I tried with these values for in_footer:

  • array( 'strategy' => 'blocking', 'in_footer' => true ) - both scripts output in footer, neither deferred
  • array( 'strategy' => 'defer', 'in_footer' => true ) - script a in header, script b in footer, both deferred
  • true - both scripts output in footer, neither deferred
  • false - script a and b in header, neither deferred

That final scenario is the worst case, but is consistent with current behavior and is expected and I feel acceptable.

#6 @adamsilverstein
14 months ago

  • Keywords needs-unit-tests added

This ticket was mentioned in Slack in #core-performance by adamsilverstein. View the logs.


14 months ago

#8 @adamsilverstein
14 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

58599.2.diff includes one slight improvement and adds unit tests

#9 @adamsilverstein
14 months ago

58599.3.diff includes all of the improvements from the PR (thanks @westonruter!)

#10 @adamsilverstein
14 months ago

Curious what other think about including this in 6.4 this late in the release cycle? It fixes a bug introduced in 6.3 and ideally we would consider back-porting to 6.3 as well. Since it only affects those using the new strategy feature, it feels pretty safe to add.

#11 @flixos90
14 months ago

Given the current state of the PR and the detailed test coverage, I am onboard with including it, given that this addresses a problem introduced in 6.3. I think we should try to commit today though to have 2 more days of "headroom" before RC1.

#12 @westonruter
14 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@westonruter commented on PR #5473:


14 months ago
#13

As a sanity check, I tried commenting out the changes to do_items and _two_ of the test cases failed:

  1. footer-blocking-dependent-of-delayed-head-script
  2. all-dependents-in-footer-with-one-blocking

These test cases did not fail:

  1. head-blocking-dependent-of-delayed-head-script
  2. delayed-footer-dependent-of-delayed-head-script
  3. delayed-dependent-in-header-and-delayed-dependents-in-footer
  4. blocking-dependents-in-head-and-footer

I looked at the test cases and this seems correct.

#14 @westonruter
14 months ago

  • Keywords commit added

#15 @westonruter
14 months ago

  • Owner changed from adamsilverstein to westonruter
  • Status changed from assigned to accepted

#16 @westonruter
14 months ago

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

In 56933:

Script Loader: Move delayed head script to footer when there is a blocking footer dependent.

This prevents a performance regression when a blocking script is enqueued in the footer which depends on a delayed script in the head (with async or defer). In order to preserve the execution order, a delayed dependency must fall back to blocking when there is a blocking dependent. But since it was originally delayed (and thus executes similarly to a footer script), it does not need to be in the head and can be moved to the footer. This prevents blocking the critical rendering path.

Props adamsilverstein, westonruter, flixos90.
Fixes #59599.
See #12009.

@westonruter commented on PR #5473:


14 months ago
#17

Committed in r56933.

#18 @adamsilverstein
14 months ago

@flixos90 or @westonruter -what are your thoughts on back-porting this to 6.3? I'm not sure how we usually handle back porting of minor bugs like this.

#19 @westonruter
14 months ago

I believe that Felix will say that a minor release would be reserved for a critical bug fix for an issue introduced in the major release. I don't think this rises to the level of critical as it wasn't even a known issue until the very end of the 6.4 development cycle.

This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.


14 months ago

#21 @adamsilverstein
14 months ago

I believe that Felix will say that a minor release would be reserved for a critical bug fix for an issue introduced in the major release. I don't think this rises to the level of critical as it wasn't even a known issue until the very end of the 6.4 development cycle.

I assumed the same. I'll make sure to include a version check before moving scripts into the header with defer.

Note: See TracTickets for help on using tickets.