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

[adamsilverstein]

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.

…

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

[adamsilverstein]

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.

[adamsilverstein]

58599.2.diff​ includes one slight improvement and adds unit tests

[adamsilverstein]

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

[adamsilverstein]

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.

[flixos90]

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.

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.

[westonruter]

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.

Committed in r56933.

[adamsilverstein]

@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.

[westonruter]

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.

[adamsilverstein]

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.