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

Negative overallSavingsMs for prioritize-lcp-image #16261

Open
connorjclark opened this issue Nov 25, 2024 · 4 comments
Open

Negative overallSavingsMs for prioritize-lcp-image #16261

connorjclark opened this issue Nov 25, 2024 · 4 comments

Comments

@connorjclark
Copy link
Collaborator

For prioritize-lcp-image, here are a 1000 urls that had a negative value for this audit.

https://gist.github.com/connorjclark/b00be94efc91fe67d1c6c9e3f7c13696

source 🔒

@brendankenny
Copy link
Member

Are these HTTP Archive numbers?

One bit of information that may or may not help your efforts: some of these won't be simulation errors, but request identification errors, since both the main resource and the LCP request are identified by URL, not by request ID. lcp-image-record does a little better by also checking that the request is from the correct frame, but main-resource is checked by URL alone, not by navigation id from the trace or anything.

#16213 has a smaller issue with this: it's really only a problem when observed and simulated TTFBs are very different, which isn't a problem for HTTP Archive numbers since it only uses observed, except when a page contains itself as an iframe, in which case main-resource identifies the iframed document as the main resource and takes the TTFB from that request, sometimes resulting in observed TTFBs larger than the observed LCP.

Two other possible issues I remember:

  • Lantern uses a different main resource (I think the real root request?), which could cause more mismatches
  • Lantern had an issue where if a request was part of an orphaned request chain (no initiator path back to the main resource), it would add the chain as a direct child to the root resource, but that might not be the page that actually made the request. I don't see how that would result in a negative overallSavingsMs in this case, but who knows :)

@brendankenny
Copy link
Member

Oh yeah, so the practical suggestion :) Maybe add an additional filtering step or boolean column(s) for when there are multiple requests on that page for the main resource or the LCP resource, could save some debugging time.

@adamsilverstein
Copy link

adamsilverstein commented Nov 25, 2024

Are these HTTP Archive numbers?

Yes. This came up when I was researching the impact of the "Image Prioritizer" plugin from the WordPress Performance Team. I was trying to use the prioritize-lcp-image as one way to measure the impact of installing this plugin.

This plugin aims to improve the accuracy of both setting a high fetchpriority for the LCP image as well as lazy loading all other images. WordPress already attempts to do this server-side semantics, the plugin adds client side data to improve the accuracy.

I also tried the offscreen-images audit which worked well shows a strong correlation (improving for sites using the plugin), but when I review values for the prioritize-lcp-image audit I noticed many negative values which seemed odd.

For refrerence, here is the query I used to get URLs with negative values for overallSavingsMs:

SELECT
  date,
  client AS device,
  page,
  IF(IS_CMS(technologies, 'Image Prioritizer', ''), TRUE, FALSE) AS has_image_prioritizer,
  CAST(JSON_EXTRACT(lighthouse, '$.audits.prioritize-lcp-image.details.overallSavingsMs') AS FLOAT64)
    AS overallSavingsMs,
FROM
  `httparchive.all.pages`
WHERE
  date = '2024-10-01'
  AND IS_CMS(technologies, 'WordPress', '')
  AND is_root_page = TRUE
  AND CAST( JSON_EXTRACT(lighthouse, '$.audits.prioritize-lcp-image.details.overallSavingsMs') AS FLOAT64) < 0
ORDER BY date DESC
LIMIT 1000

Note the IS_CMS helper function limits the scope here to WordPress sites, and is also used to detect if the "Image Prioritizer" plugin is being used.

CREATE TEMPORARY FUNCTION IS_CMS(
  technologies ARRAY<STRUCT<technology STRING, categories ARRAY<STRING>, info ARRAY<STRING>>>,
  cms STRING,
  version STRING)
RETURNS BOOL
AS (
  EXISTS(
    SELECT * FROM UNNEST(technologies) AS technology,
    UNNEST(technology.info) AS info
    WHERE
      technology.technology = cms
      AND (
        version = ''
        OR ENDS_WITH(version, '.x')
          AND (STARTS_WITH(info, RTRIM(version, 'x')) OR info = RTRIM(version, '.x'))
        OR info = version)
  )
);

@adamsilverstein
Copy link

@connorjclark - I proposed a fix in #16276, let me know what you think!

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

No branches or pull requests

4 participants