Raise the loading priority of in-viewport images.

This CL attempts to improve upon the default loading priorities for images. By default, images will start at ResourceLoadPriorityVeryLow and after a layout or during a scroll, anything that's visible will be promoted up to ResourceLoadPriorityLow.

In our tests, this was a really nice win on speed-index, with a gain of ~5% on average, at ~14% at the 99th percentile.

BUG=319073
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163110

[eseidel, 2013-11-25 23:59:30]

It looks like you're performing this work even for images which have already
loaded?  That seems suboptimal.  Otherwise the idea seems OK.

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:42:
ResourceLoadPriorityOptimizer::ResourceAndVisibility::ResourceAndVisibility()
Shoud this just have one constructor and have it have 2 args with default
values?

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:48:
ResourceLoadPriorityOptimizer::ResourceAndVisibility::ResourceAndVisibility(const
ResourceLoadPriorityOptimizer::ResourceAndVisibility& o)
Again, my C++ is a bit weak here.  How is this different from the default copy
constructor?

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:86: if
(m_imageResources.find(img->identifier()) == m_imageResources.end())
I'm surprised this doesn't have a .contains()?  I guess thsi is std::map?

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
File Source/core/fetch/ResourceLoadPriorityOptimizer.h (right):

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:45: void
notifyImageResourceVisibility(ImageResource*, bool);
Bools make awful args.  We try to use Enums when possible.  Even if those enums
only ahve 2 values.  They tend to be much easier to read.

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:59: typedef std::map<unsigned
long, ResourceAndVisibility> ImageResourceMap;
I don't recall.  Is "unsigned long" actually different from "unsigned"?

Curious why not HashMap like other blink code?  We've talked about moving more
twoards std:: types, but right now I believe WTF types are preferred when
available.

https://codereview.chromium.org/86533002/diff/1/Source/core/rendering/RenderI...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/86533002/diff/1/Source/core/rendering/RenderI...
Source/core/rendering/RenderImage.cpp:596:
optimizer.notifyImageResourceVisibility(m_imageResource->cachedImage(),
viewBounds.intersects(objectBounds));
Shouldn't we only do this if the cached image hasn't loaded?

[shatch, 2013-11-26 01:35:17]

ptal


Re: It looks like you're performing this work even for images which have already
loaded?  That seems suboptimal.  Otherwise the idea seems OK.

There's a check in ResourceLoadPriorityOptimizer::notifyImageResourceVisibility
which discards the image if it's already loaded.

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:42:
ResourceLoadPriorityOptimizer::ResourceAndVisibility::ResourceAndVisibility()
On 2013/11/25 23:59:30, eseidel wrote:
> Shoud this just have one constructor and have it have 2 args with default
> values?

Both std::map and HashMap seem to want a default constructor to create empty
elements. Alternatively, I could just new each instance of ResourceAndVisibility
to get around this. wdyt?

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:48:
ResourceLoadPriorityOptimizer::ResourceAndVisibility::ResourceAndVisibility(const
ResourceLoadPriorityOptimizer::ResourceAndVisibility& o)
On 2013/11/25 23:59:30, eseidel wrote:
> Again, my C++ is a bit weak here.  How is this different from the default copy
> constructor?

Yeah I think you're right, it's not any different, removing this.

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:86: if
(m_imageResources.find(img->identifier()) == m_imageResources.end())
On 2013/11/25 23:59:30, eseidel wrote:
> I'm surprised this doesn't have a .contains()?  I guess thsi is std::map?

Yeah it was a std::map, don't believe it has a contains() function. Changed this
to HashMap per other comment anyway.

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
File Source/core/fetch/ResourceLoadPriorityOptimizer.h (right):

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:45: void
notifyImageResourceVisibility(ImageResource*, bool);
On 2013/11/25 23:59:30, eseidel wrote:
> Bools make awful args.  We try to use Enums when possible.  Even if those
enums
> only ahve 2 values.  They tend to be much easier to read.

Done.

https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:59: typedef std::map<unsigned
long, ResourceAndVisibility> ImageResourceMap;
On 2013/11/25 23:59:30, eseidel wrote:
> I don't recall.  Is "unsigned long" actually different from "unsigned"?
> 
> Curious why not HashMap like other blink code?  We've talked about moving more
> twoards std:: types, but right now I believe WTF types are preferred when
> available.

I'm not sure if they're different? A quick search revealed:
http://stackoverflow.com/questions/2099830/unsigned-keyword-in-c which claims
when "unsigned" is used by itself, a data type of "int" is assumed. Searching on
int vs long revealed that ints could theoretically be smaller than longs
(http://stackoverflow.com/questions/7456902/long-vs-int-c-c-whats-the-point).
Honestly though, I just copied the return value of Resource::identifier(). Do
you want me to change this to an unsigned?

I remember seeing a thread on that in blink-dev back in July, wasn't sure what
the current status was. I mostly just used std::map because I'm familiar with
it, but I don't have a strong opinion on the subject. Changed it to a HashMap.

https://codereview.chromium.org/86533002/diff/1/Source/core/rendering/RenderI...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/86533002/diff/1/Source/core/rendering/RenderI...
Source/core/rendering/RenderImage.cpp:596:
optimizer.notifyImageResourceVisibility(m_imageResource->cachedImage(),
viewBounds.intersects(objectBounds));
On 2013/11/25 23:59:30, eseidel wrote:
> Shouldn't we only do this if the cached image hasn't loaded?

Done.

[eseidel, 2013-11-27 20:50:46]

On 2013/11/26 01:35:17, shatch wrote:
> ptal
> 
> 
> Re: It looks like you're performing this work even for images which have
already
> loaded?  That seems suboptimal.  Otherwise the idea seems OK.
> 
> There's a check in
ResourceLoadPriorityOptimizer::notifyImageResourceVisibility
> which discards the image if it's already loaded.
> 
>
https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
> File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):
> 
>
https://codereview.chromium.org/86533002/diff/1/Source/core/fetch/ResourceLoa...
> Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:42:
> ResourceLoadPriorityOptimizer::ResourceAndVisibility::ResourceAndVisibility()
> On 2013/11/25 23:59:30, eseidel wrote:
> > Shoud this just have one constructor and have it have 2 args with default
> > values?
> 
> Both std::map and HashMap seem to want a default constructor to create empty
> elements. Alternatively, I could just new each instance of
ResourceAndVisibility
> to get around this. wdyt?

Ah.  You may need a custom HashTraits.  This area of WTF is a bit fuzzy in my
mind:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
cevans or abarth may recall better the exact crazy that is hash templates in
c++.

[eseidel, 2013-11-27 20:58:35]

lgtm

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:77: if (!img ||
img->isLoaded())
Yes!  Sorry, missed this in my first pass.

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:83: // If the resource
exists in the map, don't overwrite since it might have been visible from
Shouldn't we just always call add?  I guess you want to override in the case
where it is in the map as invisible but now we think it's visible, but not the
other way.

AddResult result = m_imageResources.add(...);
// If we already had an entry and it wasn't visible, make sure to mark it as now
visible.
if (!addResult.wasNew && status == Visible) {
   addResult.value.status = Visible;
}

I doubt it really matters either way. The two lines differing only by "add" vs.
"set" confused me at first.

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:592: if (m_imageResource->cachedImage() &&
!m_imageResource->cachedImage()->isLoaded()) {
nit: Early returns are preferred over long if blocks.

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2846: while (child) {
for (RenderObject* child = firstChild(); child; child = child->nextSibling())

is slightly fewer lines.  But either way works.  Some year we should invent an
iterator for this sort of thing. :)

[eseidel, 2013-11-27 21:01:01]

Added abarth/cevans in case they care to comment on HashMap usage and its
interaction with your constructor and if you need a HashTraits.  I never really
needed to fully understand WTF templates in webkit land since I had others to
lean on.  I really should learn that code more deeply now.

[shatch, 2013-11-27 22:39:38]

On 2013/11/27 21:01:01, eseidel wrote:
> Added abarth/cevans in case they care to comment on HashMap usage and its
> interaction with your constructor and if you need a HashTraits.  I never
really
> needed to fully understand WTF templates in webkit land since I had others to
> lean on.  I really should learn that code more deeply now.

Looks like HashTraits defines a static function emptyValue(). I can override the
default HashTraits on the HashMap and define an emptyValue() that constructs a
ResourceAndVisibility object with null parameters.

[abarth-chromium, 2013-12-02 19:34:56]

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:75: void
ResourceLoadPriorityOptimizer::notifyImageResourceVisibility(ImageResource* img,
VisibilityStatus status)
img -> image

(Please use complete words in variable names)

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceLoadPriorityOptimizer.h (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:62: typedef
WTF::HashMap<unsigned long, ResourceAndVisibility> ImageResourceMap;
Rather than using ResourceAndVisibility, you should store an
OwnPtr<ResourceAndVisibility>.  That has a couple benefits:

1) You don't need to write hash traits for ResourceAndVisibility because OwnPtr
already has them.
2) The table will use less memory.  HashMap stores the values inline, which
means the empty slots in the table are bloated by the size of the value.

Also, WTF::HashMap -> HashMap

[shatch, 2013-12-02 22:32:46]

New snapshot uploaded, ptal

Also realized I broke the promote-img-preload-priority test, but if we're
managing priorities directly now using visibility then I don't think this test
is useful anymore. Replaced it with a new "promote-img-in-viewport-priority"
test.

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:75: void
ResourceLoadPriorityOptimizer::notifyImageResourceVisibility(ImageResource* img,
VisibilityStatus status)
On 2013/12/02 19:34:56, abarth wrote:
> img -> image
> 
> (Please use complete words in variable names)

Done.

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:83: // If the resource
exists in the map, don't overwrite since it might have been visible from
On 2013/11/27 20:58:35, eseidel wrote:
> Shouldn't we just always call add?  I guess you want to override in the case
> where it is in the map as invisible but now we think it's visible, but not the
> other way.
> 
> AddResult result = m_imageResources.add(...);
> // If we already had an entry and it wasn't visible, make sure to mark it as
now
> visible.
> if (!addResult.wasNew && status == Visible) {
>    addResult.value.status = Visible;
> }
> 
> I doubt it really matters either way. The two lines differing only by "add"
vs.
> "set" confused me at first.

Reworked it a little bit to be less confusing.

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
File Source/core/fetch/ResourceLoadPriorityOptimizer.h (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/fetch/Resourc...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:62: typedef
WTF::HashMap<unsigned long, ResourceAndVisibility> ImageResourceMap;
On 2013/12/02 19:34:56, abarth wrote:
> Rather than using ResourceAndVisibility, you should store an
> OwnPtr<ResourceAndVisibility>.  That has a couple benefits:
> 
> 1) You don't need to write hash traits for ResourceAndVisibility because
OwnPtr
> already has them.
> 2) The table will use less memory.  HashMap stores the values inline, which
> means the empty slots in the table are bloated by the size of the value.
> 
> Also, WTF::HashMap -> HashMap

Cool, thanks for the explanation! Done.

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:592: if (m_imageResource->cachedImage() &&
!m_imageResource->cachedImage()->isLoaded()) {
On 2013/11/27 20:58:35, eseidel wrote:
> nit: Early returns are preferred over long if blocks.

Done. Also changed that in RenderBlock.

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/86533002/diff/40001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.cpp:2846: while (child) {
On 2013/11/27 20:58:35, eseidel wrote:
> for (RenderObject* child = firstChild(); child; child = child->nextSibling())
> 
> is slightly fewer lines.  But either way works.  Some year we should invent an
> iterator for this sort of thing. :)

Done.

[eseidel, 2013-12-03 18:59:30]

lgtm.

[commit-bot: I haz the power, 2013-12-03 19:05:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/86533002/80001

[commit-bot: I haz the power, 2013-12-03 20:54:46]

Change committed as 163110