From 3d9632a5cdfe896e6781955b341e7814abf2f760 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Wed, 14 Aug 2019 14:18:50 +0200
Subject: [PATCH] resourceloader: Avoid isKnownEmpty call for regular
 (non-embed) modules

Modules that are never embedded, and not user-specific, should not be
excluded from the loader, even if they are empty.

Doing so has two problems:

1. Modules are expected to have their changes propagated within 5 minutes
   through the startup module. This depends on the fact that in page view
   HTML we queue the module by name, regardless of its current version.

   If the module is known to be needed by a page, then we need to queue it,
   even if the current version is empty. Otherwise, cached pages will be
   missing the module, despite the older ParserOutput perfectly knowing
   already that it was needed, which can cause bugs due to HTML not
   matching the module queue it was generated with.

2. The isKnownEmpty method can sometimes require a database lookup.
   The performance team tuned this with a preloader for the subset of
   modules we need the information for (user modules and style modules).
   In 0b1a7d4c59c8395, I accidentally made the conditions nested the wrong
   way, which made it call this much more frequently.

Bug: T230260
Bug: T176159
Change-Id: I4e6af2c833c92e1277713bdd0c68953d49c4dd9d
---
 .../ResourceLoaderClientHtml.php              | 45 ++++++++++++-------
 1 file changed, 30 insertions(+), 15 deletions(-)

diff --git a/includes/resourceloader/ResourceLoaderClientHtml.php b/includes/resourceloader/ResourceLoaderClientHtml.php
index d59e1c8c9a..e17b393a57 100644
--- a/includes/resourceloader/ResourceLoaderClientHtml.php
+++ b/includes/resourceloader/ResourceLoaderClientHtml.php
@@ -143,15 +143,15 @@ class ResourceLoaderClientHtml {
 
 			$group = $module->getGroup();
 			$context = $this->getContext( $group, ResourceLoaderModule::TYPE_COMBINED );
-			if ( $module->isKnownEmpty( $context ) ) {
-				// Avoid needless request or embed for empty module
-				$data['states'][$name] = 'ready';
-				continue;
-			}
+			$shouldEmbed = $module->shouldEmbedModule( $this->context );
 
-			if ( $group === 'user' || $module->shouldEmbedModule( $this->context ) ) {
-				// Call makeLoad() to decide how to load these, instead of
-				// loading via mw.loader.load().
+			if ( ( $group === 'user' || $shouldEmbed ) && $module->isKnownEmpty( $context ) ) {
+				// This is a user-specific or embedded module, which means its output
+				// can be specific to the current page or user. As such, we can optimise
+				// the way we load it based on the current version of the module.
+				// Avoid needless embed for empty module, preset ready state.
+				$data['states'][$name] = 'ready';
+			} elseif ( $group === 'user' || $shouldEmbed ) {
 				// - For group=user: We need to provide a pre-generated load.php
 				//   url to the client that has the 'user' and 'version' parameters
 				//   filled in. Without this, the client would wrongly use the static
@@ -187,15 +187,30 @@ class ResourceLoaderClientHtml {
 
 			$group = $module->getGroup();
 			$context = $this->getContext( $group, ResourceLoaderModule::TYPE_STYLES );
-			// Avoid needless request for empty module
-			if ( !$module->isKnownEmpty( $context ) ) {
-				if ( $module->shouldEmbedModule( $this->context ) ) {
-					// Embed via style element
+			if ( $module->shouldEmbedModule( $this->context ) ) {
+				// Avoid needless embed for private embeds we know are empty.
+				// (Set "ready" state directly instead, which we do a few lines above.)
+				if ( !$module->isKnownEmpty( $context ) ) {
+					// Embed via <style> element
 					$data['embed']['styles'][] = $name;
-				} else {
-					// Load from load.php?only=styles via <link rel=stylesheet>
-					$data['styles'][] = $name;
 				}
+			// For other style modules, always request them, regardless of whether they are
+			// currently known to be empty. Because:
+			// 1. Those modules are requested in batch, so there is no extra request overhead
+			//    or extra HTML element to be avoided.
+			// 2. Checking isKnownEmpty for those can be expensive and slow down page view
+			//    generation (T230260).
+			// 3. We don't want cached HTML to vary on the current state of a module.
+			//    If the module becomes non-empty a few minutes later, it should start working
+			//    on cached HTML without requiring a purge.
+			//
+			// But, user-specific modules:
+			// * ... are used on page views not publicly cached.
+			// * ... are in their own group and thus a require a request we can avoid
+			// * ... have known-empty status preloaded by ResourceLoader.
+			} elseif ( $group !== 'user' || !$module->isKnownEmpty( $context ) ) {
+				// Load from load.php?only=styles via <link rel=stylesheet>
+				$data['styles'][] = $name;
 			}
 			$deprecation = $module->getDeprecationInformation();
 			if ( $deprecation ) {
-- 
2.20.1