Fix Resource Priorities and Scheduling (Blink Side)

This is a field trial for experimenting with several different resource scheduling changes (individually controllable):

- Increasing the priority of fonts
- Decreasing the priority of late-body scripts
- Increasing the priority of Async JS
- Reducing the contention during the initial load of head resources

Doc that describes the rationale behind all of the changes is here: https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuMlc/edit?usp=sharing

Chrome-side of the change is here: https://codereview.chromium.org/1230133005/

BUG=317785, 517902, 517904
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200445

[Pat Meenan, 2015-07-20 14:01:09]

Patchset #1 (id:1) has been deleted

[Bryan McQuade, 2015-07-22 17:51:06]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2015-07-22 17:51:07]

https://codereview.chromium.org/1246493002/diff/20001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1246493002/diff/20001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:66: // An image fetch is used to
distinguish between "early" and "late" scripts in a document
this means that if a doc contains no images we'll get this heuristic wrong,
right? not necessarily a blocker - just want to make sure i understand. though
would be nice to have a better way to identify late body scripts.

https://codereview.chromium.org/1246493002/diff/20001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:72: if (fetchMode == 0) {
this code flow is getting pretty complex. how do you feel about having 3 helper
methods one for each fetch mode, something like:

switch (fetchMode) {
case 0: return getLoadPriorityForOriginalFetchMode();
case 1: return getLoadPriorityForAltFetchMode1();
case 2: return getLoadPriorityForAltFetchMode2();
default: ASSERT(...);
}

[Pat Meenan, 2015-07-28 16:55:15]

https://codereview.chromium.org/1246493002/diff/20001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1246493002/diff/20001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:66: // An image fetch is used to
distinguish between "early" and "late" scripts in a document
On 2015/07/22 17:51:06, Bryan McQuade wrote:
> this means that if a doc contains no images we'll get this heuristic wrong,
> right? not necessarily a blocker - just want to make sure i understand. though
> would be nice to have a better way to identify late body scripts.

Correct though the failure when it doesn't detect just reverts to the current
behavior anyway and it's exceedingly rare to not have any images on a page so it
works pretty well in practice.

By using the image fetches as a deliniation it also keeps all of the code inside
of the fetch logic and we don't have to pass information from the parser through
(particularly when processing preload requests).

Document position is a bit complicated since we parse as it arrives and we don't
necessarily know how long it is (to know if we are near the top or bottom).  We
could potentially use other non-script tokens as a deliniation in the parser as
well but that will also have a bunch of edge cases.

https://codereview.chromium.org/1246493002/diff/20001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:72: if (fetchMode == 0) {
On 2015/07/22 17:51:06, Bryan McQuade wrote:
> this code flow is getting pretty complex. how do you feel about having 3
helper
> methods one for each fetch mode, something like:
> 
> switch (fetchMode) {
> case 0: return getLoadPriorityForOriginalFetchMode();
> case 1: return getLoadPriorityForAltFetchMode1();
> case 2: return getLoadPriorityForAltFetchMode2();
> default: ASSERT(...);
> }

I changed the code and folded them all back into a single switch so it's easier
to see the individual differences.

[Pat Meenan, 2015-08-07 16:06:53]

pmeenan@chromium.org changed reviewers:
+ kouhei@chromium.org

[Pat Meenan, 2015-08-07 16:06:53]

kouhei@chromium.org: PTAL when you get a chance.  This is a field trial for
several resource priority changes we are experimenting with (and seeing really
good results from).

[Bryan McQuade, 2015-08-07 17:30:40]

Looks really good. Just a few comments. The only place where I feel like the
intent is not obvious, and where an explaining comment would help, is for the
increasePriorities boolean. Not totally obvious why it's needed. Technically it
only increases some priorities but it's hard for readers of the code to
understand which ones and why. Can you add a short comment in
ResourceFetcher.cpp to explain?

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:75: bool increasePriorities =
context().fetchIncreasePriorities();
this is the only one that could probably use a comment, mostly to explain why it
needs to exist. what does it change? does that lead to changes in actual
behavior or does it only enable some of the other boolean experiments above to
work properly? also, if any of these experiments must only be run with other
experiments enabled, it'd be good to add a comment for that and audit that it's
the case here. It's probably also worth adding a comment noting that these are
temporary in order for us to identify the optimal loading strategy, after which
point the code will have less branching and become simpler.

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:81: return increasePriorities ?
ResourceLoadPriorityVeryHigh : ResourceLoadPriorityHigh;
just an idea - perhaps we could add a member method:

ResourceLoadPriority maybeIncreasePriority(ResourceLoadPriority rlp) {
  if (!context().fetchIncreasePriorities()) return;
  switch (rlp) {
    case high: return veryhigh;
    case low: return medium;
    ...
    default: ASSERT();
  }
}

Then the logic that chooses a priority at all of the call sites is simplified:
return maybeIncreasePriority(ResourceLoadPriorityHigh);

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:70: if
(!m_retrievedSettings) {
it's kind of unfortunate that we have to do this here, in addLayoutObject, in
order to save some state that's needed in a different method. I see that this is
the only place that we really have access to a settings object though.

Maybe instead we could change the updateAllImageResourcePriorities method, which
is the only method that calls updateImageResourcesWithLoadPriority, to take a
Settings object (or a Document, or whatever you think is appropriate)? There are
only 2 call sites to updateAllImageResourcePriorities. Both are in FrameView
which should be able to easily look up a Settings object.

[Pat Meenan, 2015-08-07 21:21:14]

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:75: bool increasePriorities =
context().fetchIncreasePriorities();
On 2015/08/07 17:30:40, Bryan McQuade wrote:
> this is the only one that could probably use a comment, mostly to explain why
it
> needs to exist. what does it change? does that lead to changes in actual
> behavior or does it only enable some of the other boolean experiments above to
> work properly? also, if any of these experiments must only be run with other
> experiments enabled, it'd be good to add a comment for that and audit that
it's
> the case here. It's probably also worth adding a comment noting that these are
> temporary in order for us to identify the optimal loading strategy, after
which
> point the code will have less branching and become simpler.

Added some documentation explaining it.  All of the experiments can be toggled
individually (including the "increasePriorities" one though the results are
likely to interact with each other).

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:81: return increasePriorities ?
ResourceLoadPriorityVeryHigh : ResourceLoadPriorityHigh;
On 2015/08/07 17:30:40, Bryan McQuade wrote:
> just an idea - perhaps we could add a member method:
> 
> ResourceLoadPriority maybeIncreasePriority(ResourceLoadPriority rlp) {
>   if (!context().fetchIncreasePriorities()) return;
>   switch (rlp) {
>     case high: return veryhigh;
>     case low: return medium;
>     ...
>     default: ASSERT();
>   }
> }
> 
> Then the logic that chooses a priority at all of the call sites is simplified:
> return maybeIncreasePriority(ResourceLoadPriorityHigh);

It's a little more complicated because not all of the changes are of the same
magnitude.  Hopefully it's only short term until we get the results of the field
trial and all of the logic goes back to single decisions with fixed priorities.

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:70: if
(!m_retrievedSettings) {
On 2015/08/07 17:30:40, Bryan McQuade wrote:
> it's kind of unfortunate that we have to do this here, in addLayoutObject, in
> order to save some state that's needed in a different method. I see that this
is
> the only place that we really have access to a settings object though.
> 
> Maybe instead we could change the updateAllImageResourcePriorities method,
which
> is the only method that calls updateImageResourcesWithLoadPriority, to take a
> Settings object (or a Document, or whatever you think is appropriate)? There
are
> only 2 call sites to updateAllImageResourcePriorities. Both are in FrameView
> which should be able to easily look up a Settings object.

We could but passing around the document or settings object doesn't feel that
much cleaner on the surface.  I'm assuming the code is only going to live for
the duration of the field trial and then we can just rip it out entirely.

[Pat Meenan, 2015-08-10 16:53:19]

pmeenan@chromium.org changed reviewers:
+ japhet@chromium.org

[Pat Meenan, 2015-08-10 16:54:02]

japhet@ if you get a chance could you PTAL?

[Bryan McQuade, 2015-08-10 19:27:09]

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1246493002/diff/80001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:81: return increasePriorities ?
ResourceLoadPriorityVeryHigh : ResourceLoadPriorityHigh;
Ah, interesting, didn't realize not all boosts were of the same priority.

I'm mixed on adding complexity for temporary code. It doesn't seem like the best
thing in the long term if experiments we add all add temporary complexity -
eventually it's going to cause someone to introduce a bug.

If you see ways to simplify this change in the meantime, I think that's
beneficial, but if not I think our longer term goal should be to come up with
ways to run experiments that introduce less risk/complexity, even if that
complexity is temporary.

https://codereview.chromium.org/1246493002/diff/100001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/1246493002/diff/100001/Source/core/fetch/Reso...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:108: if
(m_increaseFetchPriorities) {
can we add ASSERT(m_retrievedSettings); here? I want to make sure we don't
accidentally do the wrong thing here because
updateImageResourcesWithLoadPriority gets called before any calls to
addLayoutObject

https://codereview.chromium.org/1246493002/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1246493002/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContext.cpp:680: return !frame()->settings() ? 0 :
frame()->settings()->fetchDeferLateScripts();
can we do

return frame()->settings() && frame()->settings()->fetchDeferLateScripts():

That's more consistent with the code above.

Same for the other getters below.

[Pat Meenan, 2015-08-10 20:21:17]

As far as complexity goes, I don't see a good way to make it any cleaner without
it effectively ending up at the same place.  There are 4 different experiment
flags, some of which interact so one way or another it is going to result in
checking multiple experiment flags before setting a priority.

That said, I do think we need to make sure we're really good about keeping
experiments around only as long as necessary to decide on a result and then
delete the various branching in the code, otherwise it is going to become
unmaintainable very quickly.

https://codereview.chromium.org/1246493002/diff/100001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/1246493002/diff/100001/Source/core/fetch/Reso...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:108: if
(m_increaseFetchPriorities) {
On 2015/08/10 19:27:09, Bryan McQuade wrote:
> can we add ASSERT(m_retrievedSettings); here? I want to make sure we don't
> accidentally do the wrong thing here because
> updateImageResourcesWithLoadPriority gets called before any calls to
> addLayoutObject

Done.

https://codereview.chromium.org/1246493002/diff/100001/Source/core/loader/Fra...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1246493002/diff/100001/Source/core/loader/Fra...
Source/core/loader/FrameFetchContext.cpp:680: return !frame()->settings() ? 0 :
frame()->settings()->fetchDeferLateScripts();
On 2015/08/10 19:27:09, Bryan McQuade wrote:
> can we do
> 
> return frame()->settings() && frame()->settings()->fetchDeferLateScripts():
> 
> That's more consistent with the code above.
> 
> Same for the other getters below.

Oh yeah, thanks.  Left over from when it was an int setting for mode 1 and 2. 
Done.

[kouhei (in TOK), 2015-08-11 01:40:34]

Wow. Looks great to me. 

I'm also a bit worried about m_imageFetched, but I'm not sure of its alternative
either.
QQ: If we have a <style> div { background-image: url("foobar.jpg") } </style>
inside <head>, would the <script> tags after it considered "late" scripts?

[kouhei (in TOK), 2015-08-11 03:48:39]

kouhei@chromium.org changed reviewers:
+ ksakamoto@google.com

[kouhei (in TOK), 2015-08-11 03:48:40]

+ksakamoto: FYI for font loading prioritization.

[Bryan McQuade, 2015-08-11 13:05:13]

Basically LGTM with these comments addressed (though I'm not a committer).

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
Source/core/fetch/ResourceFetcher.cpp:99: if (increasePriorities &&
!request.forPreload())
do you only want to elevate the priority of parser-blocking scripts if
increasePriorities is also true? if so, this can be simplified to:
if (increasePriorities && !request.forPreload())
    return  ResourceLoadPriorityVeryHigh;

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
Source/core/fetch/ResourceFetcher.cpp:101: // Early scripts discovered by the
preload scanner.
note that since your if test above looks at both forPreload() *and*
increasePriorities, this means that non-preloaded scripts will fall through to
this case if increasePriorities is not true (is that your intended behavior?)
and thus this comment isn't universally correct: at this point you are handling
either early scripts discovered by the preload scanner, or parser-blocking
scripts when increasePriorities is false. maybe we can modify the logic
slightly, or the comments, to make this clearer.

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:108:
ASSERT(m_retrievedSettings);
I think this assert is important given that we depend on the value having been
read and set here, but are we sure this will never fire? can
updateImageResourcesWithLoadPriority ever get called without a prior call to
addLayoutObject?

[Pat Meenan, 2015-08-11 13:14:38]

On 2015/08/11 01:40:34, kouhei wrote:
> Wow. Looks great to me. 
> 
> I'm also a bit worried about m_imageFetched, but I'm not sure of its
alternative
> either.
> QQ: If we have a <style> div { background-image: url("foobar.jpg") } </style>
> inside <head>, would the <script> tags after it considered "late" scripts?

At least as things stand right now the background image in a style block won't
be fetched until the style is actually applied to an element which is well after
we got into the body and hit layout.  Some flavor of that will have to be the
case even if we eventually build a css pre-parser to avoid downloading resources
that aren't used on the page (in which case it will be easy enough to filter
those out).

The only case that causes a "problem" that comes to mind is one where the page
doesn't use foreground images (maybe it uses background images for a lot of
elements or none at all) and there is js at the end of the page.  That case just
devolves into the existing implementation though so it wouldn't be any worse
than things are today.

If the experiment looks as good as we think it will then it makes sense to
evolve the logic that determines what script to load when but I'm comfortable
that the proposed implementation solves the vast majority of cases and is about
as simple as it gets.

[Pat Meenan, 2015-08-11 14:18:40]

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
Source/core/fetch/ResourceFetcher.cpp:99: if (increasePriorities &&
!request.forPreload())
On 2015/08/11 13:05:12, Bryan McQuade wrote:
> do you only want to elevate the priority of parser-blocking scripts if
> increasePriorities is also true? if so, this can be simplified to:
> if (increasePriorities && !request.forPreload())
>     return  ResourceLoadPriorityVeryHigh;
> 

I flipped it around to be consistent with the comment so ALL non-preload scripts
will trigger here.  It keeps the complexity of the different returns depending
on the state of increasePriorities but it will be cleaner than the other way
around when a decision is made.

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
Source/core/fetch/ResourceFetcher.cpp:101: // Early scripts discovered by the
preload scanner.
On 2015/08/11 13:05:12, Bryan McQuade wrote:
> note that since your if test above looks at both forPreload() *and*
> increasePriorities, this means that non-preloaded scripts will fall through to
> this case if increasePriorities is not true (is that your intended behavior?)
> and thus this comment isn't universally correct: at this point you are
handling
> either early scripts discovered by the preload scanner, or parser-blocking
> scripts when increasePriorities is false. maybe we can modify the logic
> slightly, or the comments, to make this clearer.

Cleaned up the check above so only preload scanner scripts reach this point.  It
does still include late-body scripts if deferLateScripts isn't set though so I
also updated the comment (will reduce to only being early scripts when the
experiment is complete).

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:108:
ASSERT(m_retrievedSettings);
On 2015/08/11 13:05:12, Bryan McQuade wrote:
> I think this assert is important given that we depend on the value having been
> read and set here, but are we sure this will never fire? can
> updateImageResourcesWithLoadPriority ever get called without a prior call to
> addLayoutObject?

I'm not sure the ASSERT is even needed.  updateImageResourcesWithLoadPriority is
private and is only called from the updateAllImageResourcePriorities function
above.  updateAllImageResourcePriorities starts with an empty list of image
resources and walks the layout objects to build the list of imageResources. 
It's impossible for there to be any elements in m_imageResources without
addLayoutObject having been called.

I actually removed the assert because of that but I'd be willing to put it back
in if you think it's necessary to protect against though someone would have to
radically change the logic for it to be needed.

[Bryan McQuade, 2015-08-11 14:40:02]

On 2015/08/11 14:18:40, Pat Meenan wrote:
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> File Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> Source/core/fetch/ResourceFetcher.cpp:99: if (increasePriorities &&
> !request.forPreload())
> On 2015/08/11 13:05:12, Bryan McQuade wrote:
> > do you only want to elevate the priority of parser-blocking scripts if
> > increasePriorities is also true? if so, this can be simplified to:
> > if (increasePriorities && !request.forPreload())
> >     return  ResourceLoadPriorityVeryHigh;
> > 
> 
> I flipped it around to be consistent with the comment so ALL non-preload
scripts
> will trigger here.  It keeps the complexity of the different returns depending
> on the state of increasePriorities but it will be cleaner than the other way
> around when a decision is made.
> 
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> Source/core/fetch/ResourceFetcher.cpp:101: // Early scripts discovered by the
> preload scanner.
> On 2015/08/11 13:05:12, Bryan McQuade wrote:
> > note that since your if test above looks at both forPreload() *and*
> > increasePriorities, this means that non-preloaded scripts will fall through
to
> > this case if increasePriorities is not true (is that your intended
behavior?)
> > and thus this comment isn't universally correct: at this point you are
> handling
> > either early scripts discovered by the preload scanner, or parser-blocking
> > scripts when increasePriorities is false. maybe we can modify the logic
> > slightly, or the comments, to make this clearer.
> 
> Cleaned up the check above so only preload scanner scripts reach this point. 
It
> does still include late-body scripts if deferLateScripts isn't set though so I
> also updated the comment (will reduce to only being early scripts when the
> experiment is complete).
> 
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):
> 
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:108:
> ASSERT(m_retrievedSettings);
> On 2015/08/11 13:05:12, Bryan McQuade wrote:
> > I think this assert is important given that we depend on the value having
been
> > read and set here, but are we sure this will never fire? can
> > updateImageResourcesWithLoadPriority ever get called without a prior call to
> > addLayoutObject?
> 
> I'm not sure the ASSERT is even needed.  updateImageResourcesWithLoadPriority
is
> private and is only called from the updateAllImageResourcePriorities function
> above.  updateAllImageResourcePriorities starts with an empty list of image
> resources and walks the layout objects to build the list of imageResources. 
> It's impossible for there to be any elements in m_imageResources without
> addLayoutObject having been called.
> 
> I actually removed the assert because of that but I'd be willing to put it
back
> in if you think it's necessary to protect against though someone would have to
> radically change the logic for it to be needed.

Great, thanks for investigating the code flow. I still do think having an
ASSERT, or if available a debug version of ASSERT, is nice here - gives me more
confidence that the code is working correctly. When we're debugging field trial
results, I like having fewer things to worry about potentially working
incorrectly, and having the assertion allows us to be confident that this will
always do the right thing.

[Pat Meenan, 2015-08-11 16:18:36]

On 2015/08/11 14:40:02, Bryan McQuade wrote:
> On 2015/08/11 14:18:40, Pat Meenan wrote:
> >
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> > File Source/core/fetch/ResourceFetcher.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> > Source/core/fetch/ResourceFetcher.cpp:99: if (increasePriorities &&
> > !request.forPreload())
> > On 2015/08/11 13:05:12, Bryan McQuade wrote:
> > > do you only want to elevate the priority of parser-blocking scripts if
> > > increasePriorities is also true? if so, this can be simplified to:
> > > if (increasePriorities && !request.forPreload())
> > >     return  ResourceLoadPriorityVeryHigh;
> > > 
> > 
> > I flipped it around to be consistent with the comment so ALL non-preload
> scripts
> > will trigger here.  It keeps the complexity of the different returns
depending
> > on the state of increasePriorities but it will be cleaner than the other way
> > around when a decision is made.
> > 
> >
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> > Source/core/fetch/ResourceFetcher.cpp:101: // Early scripts discovered by
the
> > preload scanner.
> > On 2015/08/11 13:05:12, Bryan McQuade wrote:
> > > note that since your if test above looks at both forPreload() *and*
> > > increasePriorities, this means that non-preloaded scripts will fall
through
> to
> > > this case if increasePriorities is not true (is that your intended
> behavior?)
> > > and thus this comment isn't universally correct: at this point you are
> > handling
> > > either early scripts discovered by the preload scanner, or parser-blocking
> > > scripts when increasePriorities is false. maybe we can modify the logic
> > > slightly, or the comments, to make this clearer.
> > 
> > Cleaned up the check above so only preload scanner scripts reach this point.

> It
> > does still include late-body scripts if deferLateScripts isn't set though so
I
> > also updated the comment (will reduce to only being early scripts when the
> > experiment is complete).
> > 
> >
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> > File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1246493002/diff/120001/Source/core/fetch/Reso...
> > Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:108:
> > ASSERT(m_retrievedSettings);
> > On 2015/08/11 13:05:12, Bryan McQuade wrote:
> > > I think this assert is important given that we depend on the value having
> been
> > > read and set here, but are we sure this will never fire? can
> > > updateImageResourcesWithLoadPriority ever get called without a prior call
to
> > > addLayoutObject?
> > 
> > I'm not sure the ASSERT is even needed. 
updateImageResourcesWithLoadPriority
> is
> > private and is only called from the updateAllImageResourcePriorities
function
> > above.  updateAllImageResourcePriorities starts with an empty list of image
> > resources and walks the layout objects to build the list of imageResources. 
> > It's impossible for there to be any elements in m_imageResources without
> > addLayoutObject having been called.
> > 
> > I actually removed the assert because of that but I'd be willing to put it
> back
> > in if you think it's necessary to protect against though someone would have
to
> > radically change the logic for it to be needed.
> 
> Great, thanks for investigating the code flow. I still do think having an
> ASSERT, or if available a debug version of ASSERT, is nice here - gives me
more
> confidence that the code is working correctly. When we're debugging field
trial
> results, I like having fewer things to worry about potentially working
> incorrectly, and having the assertion allows us to be confident that this will
> always do the right thing.

I added a comment where the setting is checked (when the layout object is added)
explaining why it is safe to only populate the setting there.  Given that the
code path to populate the image list is all private and can't be called
externally and it is all within 20-30 lines of code from each other I really do
think a full-blown ASSERT is overkill.  The assert would have to either be
inside of the for loop (only hit when there are actually image resources) or
would have to add extra branching logic for the test - neither of which feels
particularly great.

[Nate Chapin, 2015-08-11 17:41:56]

Sorry for being slow to review.

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/DEPS
File Source/core/fetch/DEPS (right):

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/DEPS...
Source/core/fetch/DEPS:5: "+core/frame/Settings.h",
This is a layering violation, please plumb this through FetchContext.

Alternately, ResourceLoadPriorityOptimizer can be moved to core/layout/.

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
Source/core/fetch/FetchContext.h:114: virtual bool fetchIncreasePriorities()
const { return false; }
This name is really vague. What priorities are being increased?

[Pat Meenan, 2015-08-11 18:50:10]

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/DEPS
File Source/core/fetch/DEPS (right):

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/DEPS...
Source/core/fetch/DEPS:5: "+core/frame/Settings.h",
On 2015/08/11 17:41:55, Nate Chapin wrote:
> This is a layering violation, please plumb this through FetchContext.
> 
> Alternately, ResourceLoadPriorityOptimizer can be moved to core/layout/.

Done.

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
Source/core/fetch/FetchContext.h:114: virtual bool fetchIncreasePriorities()
const { return false; }
On 2015/08/11 17:41:56, Nate Chapin wrote:
> This name is really vague. What priorities are being increased?

ResourceFetcher.cpp has a comment describing the full set of changes but there
are a bunch of them that all have to happen in concert (which is why they are
not individual toggles):

// Increases the priorities for CSS, Scripts, Fonts and Images all by one level
// and parser-blocking scripts and visible images by 2.
// This is used in conjunction with logic on the Chrome side to raise the
threshold
// of "layout-blocking" resources and provide a boost to resources that are
needed
// as soon as possible for something currently on the screen.

I can't think of a short but clear way to describe that but I'm happy to take
suggestions.  I could also include the comment here if that would help (or in
settings.in)

[Nate Chapin, 2015-08-12 21:13:48]

LGTM

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
Source/core/fetch/FetchContext.h:114: virtual bool fetchIncreasePriorities()
const { return false; }
On 2015/08/11 18:50:10, Pat Meenan wrote:
> On 2015/08/11 17:41:56, Nate Chapin wrote:
> > This name is really vague. What priorities are being increased?
> 
> ResourceFetcher.cpp has a comment describing the full set of changes but there
> are a bunch of them that all have to happen in concert (which is why they are
> not individual toggles):
> 
> // Increases the priorities for CSS, Scripts, Fonts and Images all by one
level
> // and parser-blocking scripts and visible images by 2.
> // This is used in conjunction with logic on the Chrome side to raise the
> threshold
> // of "layout-blocking" resources and provide a boost to resources that are
> needed
> // as soon as possible for something currently on the screen.
> 
> I can't think of a short but clear way to describe that but I'm happy to take
> suggestions.  I could also include the comment here if that would help (or in
> settings.in)

A comment in Settings.in is probably a good idea. That should be sufficient,
especially since this flag is presumably not going to be here too long?

[Pat Meenan, 2015-08-12 21:48:22]

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
File Source/core/fetch/FetchContext.h (right):

https://codereview.chromium.org/1246493002/diff/160001/Source/core/fetch/Fetc...
Source/core/fetch/FetchContext.h:114: virtual bool fetchIncreasePriorities()
const { return false; }
On 2015/08/12 21:13:47, Nate Chapin wrote:
> On 2015/08/11 18:50:10, Pat Meenan wrote:
> > On 2015/08/11 17:41:56, Nate Chapin wrote:
> > > This name is really vague. What priorities are being increased?
> > 
> > ResourceFetcher.cpp has a comment describing the full set of changes but
there
> > are a bunch of them that all have to happen in concert (which is why they
are
> > not individual toggles):
> > 
> > // Increases the priorities for CSS, Scripts, Fonts and Images all by one
> level
> > // and parser-blocking scripts and visible images by 2.
> > // This is used in conjunction with logic on the Chrome side to raise the
> > threshold
> > // of "layout-blocking" resources and provide a boost to resources that are
> > needed
> > // as soon as possible for something currently on the screen.
> > 
> > I can't think of a short but clear way to describe that but I'm happy to
take
> > suggestions.  I could also include the comment here if that would help (or
in
> > settings.in)
> 
> A comment in Settings.in is probably a good idea. That should be sufficient,
> especially since this flag is presumably not going to be here too long?

Done.  The plan is to pull all of the experiment code out and settle on the new
priority values as soon as we get enough data back from the field trial (2-3
releases would be my guess before data from stable comes back).

[Pat Meenan, 2015-08-12 21:48:32]

The CQ bit was checked by pmeenan@chromium.org

[Pat Meenan, 2015-08-12 21:48:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1246493002/#ps200001
(title: "Added comment to settings.in")

[commit-bot: I haz the power, 2015-08-12 21:49:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246493002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246493002/200001

[commit-bot: I haz the power, 2015-08-12 21:51:57]

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

[commit-bot: I haz the power, 2015-08-12 21:51:58]

Try jobs failed on following builders:
  android_blink_compile_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)
  linux_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_compile_dbg/...)
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)
  linux_chromium_gn_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[Pat Meenan, 2015-08-13 00:12:50]

The CQ bit was checked by pmeenan@chromium.org

[Pat Meenan, 2015-08-13 00:12:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1246493002/#ps220001
(title: "Rebased")

[commit-bot: I haz the power, 2015-08-13 00:13:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1246493002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1246493002/220001

[commit-bot: I haz the power, 2015-08-13 01:34:23]

Committed patchset #11 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200445