One (maybe) easy case to handle is duplicate preloads, which definitely happen in some sites when a resource is referenced multiple times in markup (usually images).

Yoav, I'd like to propose an easy stopgap: don't preload from link tags during parsing and assume the preload scanner will find them and preload them properly.

Facebook heavily uses preload. Attached is a trace of facebook where preloads are immediately fetched during the after the first chunk is tokenized, and are subsequently re-fetched when the real requests come in (i.e. they have link rel preloads just before some script, to ensure they are fetched before the extension scripts are injected).

Count how many times 7D0EF6r_jes.css is fetched.

1. The first chunk of HTML is tokenized and we pull out the <link preload> for the CSS file. At this point we have a preload request for the *actual* css but queue it, waiting for appcache.

CPU duration of all fetches: 10.716ms

2. documentElementAvaible(), we fetch the queued preloads.

CPU duration of all fetches: 7.497ms

3. Parsing the <link preload> tag. *this step could be removed*

CPU duration of all fetches: 6.391ms

4. Parsing the <link text/css> tag

Some of these fetch calls seem to be taking ~.5ms. Ideally we could remove all this overhead, but I think removing (3) is an okay first step.

What slightly scares me here is scenarios where we'd turn off the preloadScanner for some reason (I think you fixed most of the current ones, but still) and then preload links would just stop working.

Also, even without preload, the preloadScanner means that we're spending in this function ~twice as much time as we'd like.

I like the early bailout idea better.
in requestResource, if we already have Resource and it's isLinkPreload or an unused speculative preload, is there a scenario where we don't end up reusing that resource?

P.S. I think the trace didn't get attached...

I agree it is kinda scary, and you're right, solving this generally for preload scanner is a much bigger win. There looks like there are some ways a preload could end up not being used, but I'll need to take a longer look at determineRevalidationPolicy.

The problem I foresee for early bailout is all the various hooks into the embedder/subsystems in the function. E.g.
- willSendRequest
- didLoadResourceFromMemoryCache (sends an IPC to the browser processes disk cache to refresh the entry).

I'm not familiar enough with the fetch code to know if this whole function is truly necessary for the case where we actually have the resource used. One easy thing we can do is make a fast path for *preload* requests. This way at least in #3, both (2) and (3) would early exit.


+japhet for expertise here.

Also +dcheng for possible edge cases with callbacks into //content.

I ran the numbers (on a pixel C) and for FB if we solved this generally we would shave off ~30ms.

One more data point: india.com showed 50ms wins.

I will take ownership here, though I will likely need help from fetch/content experts :)

To recap a discussion offline with yoav:
1. Bailout from double preloads early. Very easy case and will win ~14ms from FB as measured by #3. This won't have huge impact across all pages because the majority win is sites that use link preloads.

2. Audit requestResource for all hooks. Evaluate whether they can be removed, given that we already made a fetch for that resource.

Would be interesting to see exactly where time is spent inside requestResource. My initial analysis/reading of the method:
* The method starts with header setting functions that don't seem necessary if we never send out the request.
* Seems like determineRevalidationPolicy already has an early bailout for preloads.
* canRequest() doesn't have a similar early bailout, and maybe it should.
* willStartLoadingResource() seems to trigger a bunch of functions reporting the load to AppCache, preparing the request, etc. I don't know it well enough to say if that's necessary for preloads.
* moveCachedNonBlockingResourceToBlocking needs to run when link preload resources are used.
* As we discussed, memoryCache->updateForAccess() should not be called for preloads reuse.
* requestLoadStarted seems to contain logic related to ResourceTiming reporting and resources that come in from MemoryCache. We need to dig further into that logic to see if it's really necessary. (I suspect it is not needed)
* startLoad() seems expensive and may not be necessary for already preloaded resources.

Mike - Can you comment on canRequest and if it needs to run when preloads are used?

obviously, if we want an early bailout on canRequest() and the header setting methods, we need to move getting the resource from memorycache to higher up in the method.

Thanks for the detailed analysis help Yoav! When I have some time I'm planning on going through those methods and seeing if it's possible (perhaps with modifications to resource) to refactor them away for preload reuse.

Note: the old issue 348655 tracks the slowness + possible optimizations for the resourceRequest path in general. I uploaded a trace there with some extra information. Note that it is for general requests (not reuse), but it shows what's slow for that case. If it's useful I can come up with a set of good traces to actually land.

Thanks for pointing me at that trace! Very helpful :)
Seems like a third of the time of requestResource in that particular case is spent inside willStartLoadingResource and another ~third is spent inside startLoad, so it probably makes sense to try and bail out earlier from those functions, if possible.

Correction to my earlier comment, which I realized while working on https://codereview.chromium.org/2319483002/ : 
`startLoad` seems to be called only when a load is actually needed (if `resourceNeedsLoad` is false, we return). So, it seems like this part is already taken care of. 

Yup. I've started a doc to start auditing the functions here. This can both serve as a reference for what could break if we early exit and how we could make the existing path faster.

PTAL. So far I've only gone through RenderFrameImpl::willSendRequest
https://docs.google.com/document/d/1hIRSqrmmGFYotSABxzL7h6noqltFR6gviRUBQ1-N9ww/edit?usp=sharing

I am starting to think we might need to split willSendRequest into two methods:
willCreateRequest and willSendRequest, where the latter is only for requests that will be send out of Blink.

willCreateRequest will allow //content and higher to modify the URL, etc. while willSendRequest will make modifications for requests that will be sent to the browser process.

I will look into this.