Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add speculative loading support #7860

Closed

Conversation

felixarntz
Copy link
Member

@felixarntz felixarntz commented Nov 21, 2024

This PR adds speculative loading support to WordPress Core, as outlined in the Trac ticket:

  • Introduce function wp_get_speculation_rules_configuration() to get the current speculation rules configuration (consisting of "mode" and "eagerness").
    • The default values for mode and eagerness are auto, which later is evaluated into whatever the current Core default is. For the initial implementation at least, that default is to "prefetch" with "conservative" eagerness.
    • A new filter wp_speculation_rules_configuration can be used to override the behavior, either to change it to something more eager, or to hard-code it to the current default to force-control it, even if Core's default should change in the future.
    • By default, speculative loading is disabled for logged-in users and for sites that do not use pretty permalinks (see below on why).
  • Introduce function wp_get_speculation_rules() to compute the full data for speculation Rules JSON output, based on the given configuration (return value from wp_get_speculation_rules_configuration()). This function is mostly a copy of the plugin's plsr_get_speculation_rules() function.
    • By default, the configuration will lead to speculative loading of all frontend URLs, while excluding patterns like /wp-admin/*, /wp-*.php, etc which should never be speculatively loaded.
    • All URLs with query parameters are also excluded, as custom query parameters may be used by plugins for custom "action links" that change state and therefore should not be speculatively loaded. If pretty permalinks are disabled, this differentiation is not possible, which is why speculative loading is disabled for those sites by default. The very small portion of sites that this applies to can still opt in to speculative loading via filter, acknowledging that this exclusion will not apply to those sites then.
    • A new filter wp_speculation_rules_href_exclude_paths can be used to add further URL patterns to exclude, relevant for plugins that may want to customize this. This filter receives the mode (prefetch or prerender) as context, which thus allows to add exclusions depending on which mode is currently used.
    • While the filter can be used to add exclusions, it cannot remove any of the default exclusions. This is because they are simply not safe to prefetch / prerender because of their dynamic nature.
    • The default configuration also excludes prefetching links that use a no-prefetch class on their anchor, as well as excludes prerendering links that use a no-prerender class.
  • Introduce function wp_print_speculation_rules() which is hooked into wp_footer to print the default speculation rules JSON, unless the wp_get_speculation_rules_configuration() returns null to indicate that speculative loading is disabled.
  • Introduce internal class WP_URL_Pattern_Prefixer, which is a direct port of the plugin's PLSR_URL_Pattern_Prefixer class that handles safe prefixing of URL patterns with WordPress base paths (like home URL, site URL, plugins directory URL etc.).

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

github-actions bot commented Nov 21, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props flixos90, swissspidy, westonruter, adamsilverstein, desrosj, mukesh27, joemcgill.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some small comments on the PR, but overall I think this looks great. I commented on the Trac ticket with some bigger picture questions/suggestions. But the PR itself seems good pending those being answered.

@felixarntz
Copy link
Member Author

felixarntz commented Feb 5, 2025

@desrosj Based on what we discussed and your related point in https://core.trac.wordpress.org/ticket/62503#comment:15, I have updated the PR in a9c52f8 to disallow use of immediate eagerness for document-level rules.

This is in line with the plugin that has never supported immediate eagerness. It makes sense to support the value because it is part of the API and certainly can be beneficial for specific URL list rules, but for the broader document rules there is probably little to no justification for using immediate and it's a good safety net to have.

@joemcgill
Copy link
Member

@felixarntz I have just a few questions on the technical approach here before diving too deep into nitpicking, as overall I think this looks pretty good.

  1. This is adding 6 helper functions in src/wp-includes/speculative-loading.php. Could some these be static methods of the class instead? E.g., the wp_is_valid_speculation_rules_* checks could be static methods. Just thinking about how with any new API we need to be careful about introducing architecture that creates a future compat burden.
  2. There's a large helper wp_get_speculation_rules() for parsing a config and returning the class as a JsonSerializable object. Had you considered passing the config directly into the constructor or a separate method of the class instead?
  3. The config that gets loaded is generated from wp_get_speculation_rules_configuration(), but if that config triggers a _doing_it_wrong it seems that this same config is loaded again from wp_get_speculation_rules_configuration() here. Can you explain why?

@felixarntz
Copy link
Member Author

felixarntz commented Feb 7, 2025

@joemcgill

This is adding 6 helper functions in src/wp-includes/speculative-loading.php. Could some these be static methods of the class instead? E.g., the wp_is_valid_speculation_rules_* checks could be static methods. Just thinking about how with any new API we need to be careful about introducing architecture that creates a future compat burden.

I'm not sure about the value of doing that. I'm not opposed, but I also don't see a benefit to it in terms of future compat. Both would be public globally available methods, so I think the future compat burden would be similar either way. I went with the functions based on how WordPress Core is largely function based. For static methods, I could see maybe the wp_is_valid_*() methods becoming static methods on WP_Speculation_Rules, but not the others because they wouldn't conceptually fit: Note that WP_Speculation_Rules is not a "wrapper" class for the entire feature, but rather a class that represents a set of specific speculation rules. More like e.g. WP_Post represents a post but is not an entry point to all post related functionality.

There's a large helper wp_get_speculation_rules() for parsing a config and returning the class as a JsonSerializable object. Had you considered passing the config directly into the constructor or a separate method of the class instead?

This relates to my above reply: The WP_Speculation_Rules class represents a set of speculation rules. It's built agnostic to any specific rules used by WordPress Core and could be instantiated several times in principle. wp_get_speculation_rules() on the other hand is for the specific speculation rules configuration that WordPress Core will use by default, that's why I don't think it would be suitable to be included in the class.

The config that gets loaded is generated from wp_get_speculation_rules_configuration(), but if that config triggers a _doing_it_wrong it seems that this same config is loaded again from wp_get_speculation_rules_configuration() here. Can you explain why?

This is present because anyone could call the wp_get_speculation_rules() and pass anything they'd like. So it's not relevant for how Core uses the function, but we need to validate that someone else doesn't pass something into it that's not supported. FWIW this function is marked private and should only be used by Core, but it's technically still publicly available. That's why this validation is present there too.

@joemcgill
Copy link
Member

Thanks for explaining your thinking, @felixarntz, particularly conceptually how you see the WP_Speculation_Rules class working.

For now, my main criticism is about how the wp_get_speculation_rules() function is designed. Currently, it looks like this function can accept a configuration and return a WP_Speculation_Rules object. In core, this is only being used to load the default configuration from wp_get_speculation_rules_configuration(). As you mentioned:

This is present because anyone could call the wp_get_speculation_rules() and pass anything they'd like. So it's not relevant for how Core uses the function, but we need to validate that someone else doesn't pass something into it that's not supported.

Is there a use case you have in mind where someone would want to use this function to generate a WP_Speculation_Rules object manually? If not, then I don't see the need to accept a configuration at all, and instead just use the config from Core internally. If there is a valid reason, then it seems undesirable that if I were to pass an invalid config that I would get Core's default config loaded in it's place.

I do think that the validation helpers conceptually fit better as methods on the class, as they are all referenced within the add_rule() method as well. In Core's usage, it seems redundant to validate the config before loading the rule, and also during execution of the add_rule() method. You could have a single static WP_Speculation_Rules::is_valid() method that accepts a config and uses private methods of the class to test the mode, source, and eagerness properties of the config to reduce the public footprint of the feature.

In fact, you could even allow a config to be passed to the constructor of the class, which would first validate the config before adding the rule (or rules) to the class, in which case wp_get_speculation_rules() could be simplified to something like this (pseudocode):

function wp_get_speculation_rules() {
  $config = wp_get_speculation_rules_configuration();

  // Include any required logic we don't want to be filterable here.

  $speculation_rules = new WP_Speculation_Rules( $config );
  
  if ( $speculation_rules->is_valid() ) {
    do_action( 'wp_load_speculation_rules', $speculation_rules );

    return $speculation_rules;
  }

  return false;
}

@felixarntz
Copy link
Member Author

@joemcgill

Is there a use case you have in mind where someone would want to use this function to generate a WP_Speculation_Rules object manually? If not, then I don't see the need to accept a configuration at all, and instead just use the config from Core internally. If there is a valid reason, then it seems undesirable that if I were to pass an invalid config that I would get Core's default config loaded in it's place.

That's fair. I did it that way for better separation of concerns and making wp_get_speculation_rules() easier to test, since you can just provide an input and run assertions on the output. But we can also achieve that by filtering the configuration, so I'm happy to update this accordingly. That way it doesn't expose an interface for something that no external developer should be doing in the first place.

I do think that the validation helpers conceptually fit better as methods on the class, as they are all referenced within the add_rule() method as well. In Core's usage, it seems redundant to validate the config before loading the rule, and also during execution of the add_rule() method. You could have a single static WP_Speculation_Rules::is_valid() method that accepts a config and uses private methods of the class to test the mode, source, and eagerness properties of the config to reduce the public footprint of the feature.

In fact, you could even allow a config to be passed to the constructor of the class

I think this mixes up multiple concerns and makes the code too messy. The concepts of "speculation rules" and "speculation rules configuration" are separate concepts:

  • Speculation rules are in line with the browser API's definition. A WordPress site can have multiple speculation rules, via the wp_load_speculation_rules action. Though by default, there will only be a single rule.
  • The speculation rules configuration concept was "invented" purely for WordPress Core's implementation, to simplify control over the default behavior for developers. The most important pieces of configuration here are the mode (whether to prefetch or prerender) and the eagerness. This could be achieved by simply modifying the default rule, but it would be unnecessarily complex because of the very flexible and verbose format the speculation rules syntax of the browser API uses. So the speculation rules configuration controls the one speculation rule that WordPress Core will add by default.

For this distinction, I think intertwining the WP_Speculation_Rules class with the wp_get_speculation_rules_configuration() function would not be the right approach.

@felixarntz
Copy link
Member Author

@joemcgill Based on your feedback, I've updated the PR:

  • 349f4c0 removes the unnecessary wp_get_speculation_rules() parameter and instead always uses the configuration.
  • 029324e moves the global validation functions to become static methods on WP_Speculation_Rules.

Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @felixarntz, the new changes address my main concerns and I'm happy with this approach for an initial commit.

Copy link

A commit was made that fixes the Trac ticket referenced in the description of this pull request.

SVN changeset: 59837
GitHub commit: 95c00d0

This PR will be closed, but please confirm the accuracy of this and reopen if there is more work to be done.

@github-actions github-actions bot closed this Feb 18, 2025
@mukeshpanchal27
Copy link
Member

Hi just notice that the performance number on GHA shows some regression https://github.com/WordPress/wordpress-develop/actions/runs/13400657032

@felixarntz
Copy link
Member Author

@mukeshpanchal27 Which specific metric(s) are you referring to?

@desrosj
Copy link
Contributor

desrosj commented Feb 26, 2025

@mukeshpanchal27 Just pinging again for clarification to make sure we can ideally address this prior to Beta 1, if necessary.

@mukeshpanchal27
Copy link
Member

Single Site Default summary

Homepage › Theme: twentytwentyone, Locale: en_US

Metric Before After Diff abs. Diff % STD MAD
timeToFirstByte 43.85 ms 45.15 ms 1.30 ms 2.88 % 1.98 ms 1.25 ms
largestContentfulPaint 95.80 ms 101.95 ms 6.15 ms 6.03 % 3.88 ms 2.40 ms
lcpMinusTtfb 51.85 ms 56.80 ms 4.95 ms 8.71 % 2.78 ms 1.20 ms
wpBeforeTemplate 14.27 ms 14.34 ms 0.07 ms 0.52 % 1.20 ms 0.60 ms
wpTemplate 27.39 ms 28.38 ms 0.98 ms 3.45 % 1.24 ms 0.73 ms
wpTotal 41.85 ms 42.98 ms 1.13 ms 2.64 % 1.97 ms 0.96 ms
wpMemoryUsage 3.55 MB 3.62 MB 0.07 MB 1.82 % 0.00 MB 0.00 MB
wpDbQueries 27 27 0 0.00 % 0 0
wpExtObjCache no no

Homepage › Theme: twentytwentyone, Locale: de_DE

Metric Before After Diff abs. Diff % STD MAD
timeToFirstByte 44.95 ms 46.80 ms 1.85 ms 3.95 % 2.46 ms 1.65 ms
largestContentfulPaint 96.95 ms 102.50 ms 5.55 ms 5.41 % 4.68 ms 2.10 ms
lcpMinusTtfb 52.15 ms 56.55 ms 4.40 ms 7.78 % 3.31 ms 1.15 ms
wpBeforeTemplate 14.54 ms 14.97 ms 0.43 ms 2.87 % 1.70 ms 0.82 ms
wpTemplate 27.59 ms 28.57 ms 0.99 ms 3.46 % 1.07 ms 0.66 ms
wpTotal 42.33 ms 43.51 ms 1.18 ms 2.71 % 2.32 ms 1.36 ms
wpMemoryUsage 3.76 MB 3.79 MB 0.03 MB 0.87 % 0.17 MB 0.17 MB
wpDbQueries 27 27 0 0.00 % 0 0
wpExtObjCache no no

The classic theme shows some regression in LCP, LCP - TTFB and wp-template per the GHA - https://github.com/WordPress/wordpress-develop/actions/runs/13400657032

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done 😃
Development

Successfully merging this pull request may close these issues.

7 participants