Don't wait for AppCache for link rel preloads

Don't wait for AppCache for link rel preloads

The preload scanner currently waits for the Application Cache to be
initialized before sending out any preloads. This is less important for link
rel preloads, which are directives to send a fetch as soon as possible.

This patch bypasses the fetch queuing for these preloads.

For link preloads in headers, we can issue these as early as the commit
point (for those which don't have a media attribute).

BUG=629420
Committed: https://crrev.com/7eacc60e1556444061f9f84bac4d82c34419a2ae
Cr-Commit-Position: refs/heads/master@{#407227}

[Charlie Harrison, 2016-07-20 03:19:40]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 03:20:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 04:54:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 04:54:07]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-07-21 19:15:58]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 19:16:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 19:35:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 19:35:27]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Charlie Harrison, 2016-07-22 03:19:29]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 03:20:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-07-22 03:47:44]

Description was changed from

==========
Don't wait for AppCache for link rel preloads

The preload scanner currently waits for the Application Cache to be
initialized before sending out any preloads. This is less important for link
rel preloads, which are directives to send a fetch as soon as possible.

This patch bypasses the fetch queuing for these preloads.

BUG=629420
==========

to

==========
Don't wait for AppCache for link rel preloads

The preload scanner currently waits for the Application Cache to be
initialized before sending out any preloads. This is less important for link
rel preloads, which are directives to send a fetch as soon as possible.

This patch bypasses the fetch queuing for these preloads.

For link preloads in headers, we can issue these as early as the commit
point (for those which don't have a media attribute).

BUG=629420
==========

[Charlie Harrison, 2016-07-22 04:19:47]

csharrison@chromium.org changed reviewers:
+ kouhei@chromium.org, yoav@yoav.ws

[Charlie Harrison, 2016-07-22 04:19:48]

kouhei@, yoav@. PTAL?

I did a bit of refactoring of prefetching link headers into DocumentLoader, too.

[kouhei (in TOK), 2016-07-22 04:29:55]

lgtm please wait for yoav@

[commit-bot: I haz the power, 2016-07-22 04:36:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 04:36:54]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-07-22 08:43:28]

Overall looks good. I don't understand the AppCache waiting and why can't we
just nuke it for all resources, though. Seems like we already "protect"
ourselves from AppCache by disabling the preloadScanner when it is present.

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:307: for
(auto& request : chunk->preloads) {
I don't understand why this waiting is necessary for *any* resource. The
preloadScanner bails out from preloading if AppCache is present. So AFAIUI if we
have preloads here, there's no AppCache.

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LinkLoader.h (right):

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LinkLoader.h:78: // Media links cannot be
preloaded until the first chunk is parsed. The rest
"well, actually" warning: their preloading is waiting until the first chunk is
tokenized, not necessarily parsed. But at the same time, we seem to be using
"parsed" all over the place (e.g. notifyPendingParsedChunks is called from
sendTokensToMainThread, where m_parsedChunkQueue is used to store tokenized
chunks...)

[Charlie Harrison, 2016-07-22 14:03:22]

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:307: for
(auto& request : chunk->preloads) {
On 2016/07/22 08:43:28, Yoav Weiss wrote:
> I don't understand why this waiting is necessary for *any* resource. The
> preloadScanner bails out from preloading if AppCache is present. So AFAIUI if
we
> have preloads here, there's no AppCache.

My understanding is that the preload scanner just looks for the manifest
attribute on the html element, but I think you can have appcache enabled even
without that check. Looking over the code right now
(WebApplicationCacheHostImpl::selectCacheWithoutManifest) it looks like this
might only happen if the document you loaded was loaded via appcache? Kouhei, do
you have more context for this?

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/LinkLoader.h (right):

https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/LinkLoader.h:78: // Media links cannot be
preloaded until the first chunk is parsed. The rest
On 2016/07/22 08:43:28, Yoav Weiss wrote:
> "well, actually" warning: their preloading is waiting until the first chunk is
> tokenized, not necessarily parsed. But at the same time, we seem to be using
> "parsed" all over the place (e.g. notifyPendingParsedChunks is called from
> sendTokensToMainThread, where m_parsedChunkQueue is used to store tokenized
> chunks...)

You're right. It's better to be clear here, and using "parsed" for tokenized
confuses me as well :) If that's the case, then why do we only load links from
headers *after* constructTreeFromCompactHTMLToken (parsing) is called?

We could move that call into notifyPendingParsedChunks if all it needs is the
chunk viewport.

[Yoav Weiss, 2016-07-22 14:09:48]

On 2016/07/22 14:03:22, csharrison wrote:
>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):
> 
>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:307: for
> (auto& request : chunk->preloads) {
> On 2016/07/22 08:43:28, Yoav Weiss wrote:
> > I don't understand why this waiting is necessary for *any* resource. The
> > preloadScanner bails out from preloading if AppCache is present. So AFAIUI
if
> we
> > have preloads here, there's no AppCache.
> 
> My understanding is that the preload scanner just looks for the manifest
> attribute on the html element, but I think you can have appcache enabled even
> without that check. Looking over the code right now
> (WebApplicationCacheHostImpl::selectCacheWithoutManifest) it looks like this
> might only happen if the document you loaded was loaded via appcache? Kouhei,
do
> you have more context for this?
> 
>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/LinkLoader.h (right):
> 
>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/LinkLoader.h:78: // Media links cannot
be
> preloaded until the first chunk is parsed. The rest
> On 2016/07/22 08:43:28, Yoav Weiss wrote:
> > "well, actually" warning: their preloading is waiting until the first chunk
is
> > tokenized, not necessarily parsed. But at the same time, we seem to be using
> > "parsed" all over the place (e.g. notifyPendingParsedChunks is called from
> > sendTokensToMainThread, where m_parsedChunkQueue is used to store tokenized
> > chunks...)
> 
> You're right. It's better to be clear here, and using "parsed" for tokenized
> confuses me as well :) If that's the case, then why do we only load links from
> headers *after* constructTreeFromCompactHTMLToken (parsing) is called?
> 
> We could move that call into notifyPendingParsedChunks if all it needs is the
> chunk viewport.

Good call! We should definitely bump them up before parsing.

[Charlie Harrison, 2016-07-22 14:51:34]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 14:51:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-07-22 14:59:13]

Updated the PS so link headers with media attrs are preloaded on the main thread
ASAP.

This significantly improves performance for them, because waiting for the first
chunk to parse can (sometimes) cause big delays setting up the WindowProxy and
initializing extensions.

[Yoav Weiss, 2016-07-22 16:03:46]

https://codereview.chromium.org/2165653004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2165653004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:214: if
(!linkHeader.isEmpty()) {
Why is this wrapper needed? There's already an empty check inside
loadLinksFromHeader

[Yoav Weiss, 2016-07-22 16:17:20]

On 2016/07/22 16:03:46, Yoav Weiss wrote:
>
https://codereview.chromium.org/2165653004/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/2165653004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/DocumentLoader.cpp:214: if
> (!linkHeader.isEmpty()) {
> Why is this wrapper needed? There's already an empty check inside
> loadLinksFromHeader

LGTM % above nit (I think we don't really need this wrapper, unless there's a
good reason for it that I'm missing).

Also, let's land current AppCache avoidance for preload links and avoid waiting
for HTMLHTMLElement creation for all resources (if possible) in a followup CL.

[Charlie Harrison, 2016-07-22 16:53:25]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 16:53:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-07-22 16:54:24]

Your proposal SGTM.

https://codereview.chromium.org/2165653004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2165653004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:214: if
(!linkHeader.isEmpty()) {
On 2016/07/22 16:03:46, Yoav Weiss wrote:
> Why is this wrapper needed? There's already an empty check inside
> loadLinksFromHeader

Ah good catch. Removed. I still kept this helper function to avoid awkward
callsites reaching deep into document()->loader().

[Charlie Harrison, 2016-07-22 17:40:11]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2016-07-22 17:40:11]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-07-22 17:40:12]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org,
yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/2165653004/#ps80001
(title: "Remove empty check")

[commit-bot: I haz the power, 2016-07-22 17:40:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 19:16:14]

Description was changed from

==========
Don't wait for AppCache for link rel preloads

The preload scanner currently waits for the Application Cache to be
initialized before sending out any preloads. This is less important for link
rel preloads, which are directives to send a fetch as soon as possible.

This patch bypasses the fetch queuing for these preloads.

For link preloads in headers, we can issue these as early as the commit
point (for those which don't have a media attribute).

BUG=629420
==========

to

==========
Don't wait for AppCache for link rel preloads

The preload scanner currently waits for the Application Cache to be
initialized before sending out any preloads. This is less important for link
rel preloads, which are directives to send a fetch as soon as possible.

This patch bypasses the fetch queuing for these preloads.

For link preloads in headers, we can issue these as early as the commit
point (for those which don't have a media attribute).

BUG=629420
==========

[commit-bot: I haz the power, 2016-07-22 19:16:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-22 19:17:57]

Description was changed from

==========
Don't wait for AppCache for link rel preloads

The preload scanner currently waits for the Application Cache to be
initialized before sending out any preloads. This is less important for link
rel preloads, which are directives to send a fetch as soon as possible.

This patch bypasses the fetch queuing for these preloads.

For link preloads in headers, we can issue these as early as the commit
point (for those which don't have a media attribute).

BUG=629420
==========

to

==========
Don't wait for AppCache for link rel preloads

The preload scanner currently waits for the Application Cache to be
initialized before sending out any preloads. This is less important for link
rel preloads, which are directives to send a fetch as soon as possible.

This patch bypasses the fetch queuing for these preloads.

For link preloads in headers, we can issue these as early as the commit
point (for those which don't have a media attribute).

BUG=629420

Committed: https://crrev.com/7eacc60e1556444061f9f84bac4d82c34419a2ae
Cr-Commit-Position: refs/heads/master@{#407227}
==========

[commit-bot: I haz the power, 2016-07-22 19:17:58]

Patchset 5 (id:??) landed as
https://crrev.com/7eacc60e1556444061f9f84bac4d82c34419a2ae
Cr-Commit-Position: refs/heads/master@{#407227}

[kouhei (in TOK), 2016-07-25 01:13:04]

Great work, csharrison!

On 2016/07/22 14:03:22, csharrison wrote:
>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
(right):
> 
>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:307: for
> (auto& request : chunk->preloads) {
> On 2016/07/22 08:43:28, Yoav Weiss wrote:
> > I don't understand why this waiting is necessary for *any* resource. The
> > preloadScanner bails out from preloading if AppCache is present. So AFAIUI
if
> we
> > have preloads here, there's no AppCache.
> 
> My understanding is that the preload scanner just looks for the manifest
> attribute on the html element, but I think you can have appcache enabled even
> without that check. Looking over the code right now
> (WebApplicationCacheHostImpl::selectCacheWithoutManifest) it looks like this
> might only happen if the document you loaded was loaded via appcache? Kouhei,
do
> you have more context for this?
Your understanding is correct. :)

>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/LinkLoader.h (right):
> 
>
https://codereview.chromium.org/2165653004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/LinkLoader.h:78: // Media links cannot
be
> preloaded until the first chunk is parsed. The rest
> On 2016/07/22 08:43:28, Yoav Weiss wrote:
> > "well, actually" warning: their preloading is waiting until the first chunk
is
> > tokenized, not necessarily parsed. But at the same time, we seem to be using
> > "parsed" all over the place (e.g. notifyPendingParsedChunks is called from
> > sendTokensToMainThread, where m_parsedChunkQueue is used to store tokenized
> > chunks...)
> 
> You're right. It's better to be clear here, and using "parsed" for tokenized
> confuses me as well :) If that's the case, then why do we only load links from
> headers *after* constructTreeFromCompactHTMLToken (parsing) is called?
> 
> We could move that call into notifyPendingParsedChunks if all it needs is the
> chunk viewport.

Let's rename those.