Refactor optimization logic in Optimization Detective #1172

westonruter · 2024-04-23T18:14:36Z

This PR refactors the logic to facilitate extracting optimization logic into separate dependent plugins, for example Image Prioritizer (#1088).

To review the changes here, I suggest reviewing commit-by-commit, as I've carefully rebased changes to keep each commit as atomic as possible.

The biggest change is the introduction of the OD_Preload_Link_Collection class. Instead of building up an array data structure while walking over the document and then manually constructing HTML link tags once iteration is over, the use of this new class makes the logic much clearer as the pending preload links are added during iteration in a well-defined object interface.

Additionally, instead of passing around an array of LCP element data arrays keyed by the minimum viewport widths (via od_get_lcp_elements_by_minimum_viewport_widths()), this logic is now moved over to a more logical place as a get_lcp_element method on the OD_URL_Metrics_Group class. The minimum viewport and maximum viewport can be obtained via the methods on the OD_URL_Metrics_Group instance. This simplifies construction of the preload links, as each group knows its minimum and maximum viewport width, without having to iterate over other keys to discover which one is the next-largest.

Also, there was a somewhat subtle distinction when obtaining the LCP element data for a given viewport group: it could return an array, false, or null. The array case meant that there was an LCP element found. The false case meant that there were URL Metrics gathered, but no supported LCP element was present. Lastly, the null meant that there was no data available at all. In c6681cf and 07e19c1 I've merged these false and null states into just null. The logic was going ahead and preloading an LCP element from a smaller viewport group when the next-largest viewport group was missing data. This is now undone so that it will only preload images which are actually detected as being LCP elements.

Work in this PR leads into #1235 which will address #1088.

westonruter · 2024-05-21T21:46:10Z

Last commit before heavily rebasing: 4bc0540

westonruter · 2024-05-21T22:44:39Z

plugins/optimization-detective/class-od-html-tag-walker.php

+		if ( $this->did_start_walking ) {
+			throw new Exception( esc_html__( 'Open tags may only be iterated over once per instance.', 'optimization-detective' ) );
+		}
+		$this->did_start_walking = true;


Since WP_HTML_Tag_Processor doesn't rewind after iterating over tags in the document, it's important that open_tags() also not support being invoked multiple times to walk over the document.

westonruter · 2024-05-21T22:45:18Z

plugins/optimization-detective/class-od-html-tag-walker.php

+		if ( null === $this->current_xpath ) {
+			$this->current_xpath = '';
+			foreach ( $this->get_breadcrumbs() as list( $tag_name, $index ) ) {
+				$this->current_xpath .= sprintf( '/*[%d][self::%s]', $index + 1, $tag_name );
+			}
 		}


Just a small optimization in case get_xpath() is called multiple times when visiting the same tag.

westonruter · 2024-05-21T22:46:05Z

plugins/optimization-detective/class-od-html-tag-walker.php

+	public function get_tag(): ?string {
+		return $this->processor->get_tag();
+	}


This ensures that when passing the OD_HTML_Tag_Walker instance around, the current tag can always be obtained even when not looping over open_tags().

westonruter · 2024-05-21T22:48:15Z

plugins/optimization-detective/optimization.php

-			preg_match( '/background(-image)?\s*:[^;]*?url\(\s*[\'"]?(?<background_image>.+?)[\'"]?\s*\)/', (string) $style, $matches )
+			preg_match( '/background(-image)?\s*:[^;]*?url\(\s*[\'"]?\s*(?<background_image>.+?)\s*[\'"]?\s*\)/', (string) $style, $matches )
 			&&
-			! str_starts_with( $matches['background_image'], 'data:' )
+			! $is_data_url( $matches['background_image'] )


Just a little hardening bugfix to account for whitespace around the URL or, more unlikely, the scheme to be DATA: which does work in browsers.

westonruter · 2024-05-21T22:49:00Z

plugins/optimization-detective/optimization.php

-		$common_lcp_element = current( $lcp_elements_by_minimum_viewport_widths );
+		$common_lcp_xpath = key( $groups_by_lcp_element_xpath );


Just capturing the data we need (the XPath) as opposed to the entire element.

westonruter · 2024-05-21T22:50:21Z

plugins/optimization-detective/optimization.php

-	// If there were any LCP elements captured in URL Metrics that no longer exist in the document, we need to behave as
-	// if they didn't exist in the first place as there is nothing that can be preloaded.
-	foreach ( array_keys( $lcp_element_minimum_viewport_widths_by_xpath ) as $xpath ) {
-		if ( empty( $detected_lcp_element_xpaths[ $xpath ] ) ) {
-			foreach ( $lcp_element_minimum_viewport_widths_by_xpath[ $xpath ] as $minimum_viewport_width ) {
-				$lcp_elements_by_minimum_viewport_widths[ $minimum_viewport_width ] = false;
-			}
-		}
-	}


This is not needed anymore now that preload links are added during iteration above.

westonruter · 2024-05-21T22:51:32Z

plugins/optimization-detective/storage/data.php

- *                                                the array shape is actually an ElementData from OD_URL_Metric but
- *                                                PHPStan does not support importing a type onto a function.
- */
-function od_get_lcp_elements_by_minimum_viewport_widths( OD_URL_Metrics_Group_Collection $group_collection ): array {


This logic was moved to OD_URL_Metrics_Group::get_lcp_element().

westonruter · 2024-05-21T22:52:26Z

plugins/optimization-detective/storage/data.php

-	// Now merge the breakpoints when there is an LCP element common between them.
-	$prev_lcp_element = null;
-	return array_filter(
-		$lcp_element_by_viewport_minimum_width,
-		static function ( $lcp_element ) use ( &$prev_lcp_element ) {
-			$include = (
-				// First element in list.
-				null === $prev_lcp_element
-				||
-				( is_array( $prev_lcp_element ) && is_array( $lcp_element )
-					?
-					// This breakpoint and previous breakpoint had LCP element, and they were not the same element.
-					$prev_lcp_element['xpath'] !== $lcp_element['xpath']
-					:
-					// This LCP element and the last LCP element were not the same. In this case, either variable may be
-					// false or an array, but both cannot be an array. If both are false, we don't want to include since
-					// it is the same. If one is an array and the other is false, then do want to include because this
-					// indicates a difference at this breakpoint.
-					$prev_lcp_element !== $lcp_element
-				)
-			);
-			$prev_lcp_element = $lcp_element;
-			return $include;
-		}
-	);


Now handled by array_reduce() in OD_Preload_Link_Collection::get_adjacent_deduplicated_links()

westonruter · 2024-05-21T22:53:55Z

tests/plugins/optimization-detective/storage/data-tests.php

-	 * @throws OD_Data_Validation_Exception When failing to instantiate a URL metric.
-	 * @return array<string, mixed> Data.
-	 */
-	public function data_provider_test_get_lcp_elements_by_minimum_viewport_widths(): array {


Tests moved to a5b4699.

…et_lcp_element

westonruter · 2024-05-21T23:18:15Z

plugins/optimization-detective/optimization.php

- * @param array<int, array{background_image?: string, img_attributes?: array{src?: string, srcset?: string, sizes?: string, crossorigin?: string}}|false> $lcp_elements_by_minimum_viewport_widths LCP elements keyed by minimum viewport width, amended with element details.
- * @return string Markup for zero or more preload link tags.
- */
-function od_construct_preload_links( array $lcp_elements_by_minimum_viewport_widths ): string {


Logic moved to OD_Preload_Link_Collection class.

westonruter · 2024-05-21T23:21:45Z