Re: Lazy loading resource requests with changing client hints

[Kouhei Ueno]

+loading-dev, hiroshige

Hi Philip,

Thanks for reaching out!

How about we defer computing Sec-CH-Width header value until the very end instead?

I'm happy to look into this in more detail but would you point me to where the header value is originally computed+set? I couldn't find it from a quick code search.

On Mon, May 13, 2024 at 8:27 AM Philip Rogers <p...@google.com> wrote:

Hi Kouhei,

Could you help me with a platform/loading design question for https://crbug.com/335630145?

Lazy loaded image resources are created early in ResourceFetcher::RequestResource, but may not actually load (also via ResourceFetcher::RequestResource) until much later. We support the sec-ch-width header which is set to the layout width of the image. At the time we actually send the first lazy load image request in ResourceFetcher::RequestResource, we can have two sec-ch-width values:

  1. resource->GetResourceRequest() has a sec-ch-width header set based on the initial layout width.

  2. params.MutableResourceRequest() has a sec-ch-width header set to the current layout width.

We have a bug because #1 is used for the request and sends the out-dated layout width. This hacky patch fixes the bug, but seems out of place. Do you know if there is a better approach to fix this bug?

--

kouhei

[Philip Rogers]

(re-sending from pdr@chromium.org)

Here's the stack for setting the Sec-CH-Width header:
  BaseFetchContext::AddClientHintsIfNecessary
  FrameFetchContext::AddClientHintsIfNecessary
  FrameFetchContext::PopulateResourceRequest
  PrepareResourceRequest
  ResourceFetcher::PrepareRequest
  ResourceFetcher::RequestResource
  ImageResource::Fetch
  ImageResourceContent::Fetch
  ImageLoader::DoUpdateFromElement

In the auto sizes lazy loaded image case, an initial request will be created and stored in the memory cache, but won't be sent to the network. If we later need to request the lazy loaded image, we will make another call to ImageLoader::DoUpdateFromElement, which will create another request with the updated sec-ch-width header, but we will then send the original cached request to the network.

Do we need to store deferred resources in the memory cache? I wonder if an approach like this patch would be a good path forward (note: currently fails some tests)?

[Kouhei Ueno]

Thank you! Sorry I ran out of time today and will further follow up later this week.

On Tue, May 14, 2024 at 6:31 AM Philip Rogers <p...@chromium.org> wrote:

(re-sending from pdr@chromium.org)

Here's the stack for setting the Sec-CH-Width header:
  BaseFetchContext::AddClientHintsIfNecessary
  FrameFetchContext::AddClientHintsIfNecessary
  FrameFetchContext::PopulateResourceRequest
  PrepareResourceRequest
  ResourceFetcher::PrepareRequest
  ResourceFetcher::RequestResource
  ImageResource::Fetch
  ImageResourceContent::Fetch
  ImageLoader::DoUpdateFromElement

In the auto sizes lazy loaded image case, an initial request will be created and stored in the memory cache, but won't be sent to the network. If we later need to request the lazy loaded image, we will make another call to ImageLoader::DoUpdateFromElement, which will create another request with the updated sec-ch-width header, but we will then send the original cached request to the network.

Thank you for the pointer + articulating the problem, I'll see if there's a good place to populate the client hints header when the request is undeferred+armed.

 

Do we need to store deferred resources in the memory cache? I wonder if an approach like this patch would be a good path forward (note: currently fails some tests)?

I think we do need to store deferred image resources in the memory cache, otherwise they will be duplicated fetches/responses. If there are multiple img tags against the same url, all deferred, we will duplicate ImageResources for them.

 

On Mon, May 13, 2024 at 12:06 AM Kouhei Ueno <kou...@google.com> wrote:

+loading-dev, hiroshige

Hi Philip,

Thanks for reaching out!

How about we defer computing Sec-CH-Width header value until the very end instead?

I'm happy to look into this in more detail but would you point me to where the header value is originally computed+set? I couldn't find it from a quick code search.

On Mon, May 13, 2024 at 8:27 AM Philip Rogers <p...@google.com> wrote:

Hi Kouhei,

Could you help me with a platform/loading design question for https://crbug.com/335630145?

Lazy loaded image resources are created early in ResourceFetcher::RequestResource, but may not actually load (also via ResourceFetcher::RequestResource) until much later. We support the sec-ch-width header which is set to the layout width of the image. At the time we actually send the first lazy load image request in ResourceFetcher::RequestResource, we can have two sec-ch-width values:

  1. resource->GetResourceRequest() has a sec-ch-width header set based on the initial layout width.

  2. params.MutableResourceRequest() has a sec-ch-width header set to the current layout width.

We have a bug because #1 is used for the request and sends the out-dated layout width. This hacky patch fixes the bug, but seems out of place. Do you know if there is a better approach to fix this bug?

--

kouhei

--

kouhei

[Kouhei Ueno]

I have made some progress but haven't tested the idea yet.

pdr: (sorry i should have asked earlier) what would be the timeline/urgency/severity of the bug?

Overall I feel like we are doing way too much work on the request *before* we even perform matching against MemoryCache.

I'd like to try how far we can defer the FrameFetchContext::PopulateResourceRequest call. I'd like to try taking it at minimum after `ResourceFetcher::StartLoad`, but even that is quite too soon - we have a few layers of deferring mechanisms that could still kick in after that, but it might be a compromise for the WPT.

Maybe moving FrameFetchContext::PopulateResourceRequest on one CL is too challenging, in that case we can introduce an intermediate state where it would only move the FrameFetchContext::AddClientHintsIfNecessary call.

+ area expert @Takashi Toyoshima since he just started looking into img.src= optimization.

--

kouhei

[Kouhei Ueno]

I discussed this with @Tsuyoshi Horo @Kenichi Ishibashi @Yoshisato Yanagisawa today.

Now we feel https://chromium-review.googlesource.com/c/chromium/src/+/5533433 is probably closest to the direction we envision right now.

I think we would like to try optimizing the resource request creation logic to keep the current semantics.
Since the work will take time, I'm good with you proceeding with the stop-gap solution for now.

Do you want to unabandon the CL and land?

--

kouhei

[Philip Rogers]

Thank you very much for investigating this! I will clean that patch up and send it out for review.