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.

This was committed as r163110, but rolled back due to performance regressions (https://code.google.com/p/chromium/issues/detail?id=326165) in r163362. Link to previous review: https://codereview.chromium.org/86533002/

From profiling the code, the regression is almost completely from the extra time spent in RenderBox::absoluteContentBox called from RenderBlock::updateStyleImageLoadingPriorities. Refactored the code to early out before calling that.


Results of performance tests:

Format:
1st line: Original CL          <-- Regression
2nd line: Original CL Disabled <-- This should be target performance
3rd line: This CL              <-- Hopefully roughly the same as (2)

Pages: [floats_50_100.html]
1. Avg floats_50_100: 229.050280ms
2. Avg floats_50_100: 187.935380ms
3. Avg floats_50_100: 190.970400ms


Pages: [fixed-grid-lots-of-data.html]
1. Avg fixed-grid-lots-of-data: 727.259726runs/s
2. Avg fixed-grid-lots-of-data: 1404.643989runs/s
3. Avg fixed-grid-lots-of-data: 1419.710566runs/s


Pages: [ChangingClassNameShadowDOM.html]
1. Avg ChangingClassNameShadowDOM: 71.947800ms
2. Avg ChangingClassNameShadowDOM: 61.533920ms
3. Avg ChangingClassNameShadowDOM: 61.871240ms


page_sets/typical_25.json
1. Avg warm_times: 685.391111ms
2. Avg warm_times: 644.857778ms
3. Avg warm_times: 649.995556ms


http___ynet.co.il_
1. Avg warm_times: 753.111111ms
2. Avg warm_times: 677.000000ms
3. Avg warm_times: 654.666667ms


page_sets/intl_es_fr_pt-BR.json
1. Avg warm_times: 406.922222ms
2. Avg warm_times: 397.788889ms
3. Avg warm_times: 399.477778ms


robohornet_pro
1. *RESULT Total: Total= 5041 ms
2. *RESULT Total: Total= 3692 ms
3. *RESULT Total: Total= 3514 ms



BUG=319073, 326165, 326483
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163563

[shatch, 2013-12-07 03:16:15]

https://codereview.chromium.org/109153003/diff/20001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/109153003/diff/20001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1681: if (m_frame->document() &&
m_frame->document()->renderer()) {
There was a crash on mac on FrameView::scrollPositionChanged during destruction
of a FrameView. Couldn't reproduce it locally on linux (don't have a mac), so I
forced a scrollPositionChanged from the destructor of FrameView and ended up
crashing with a NULL renderer() from m_frame->document()->renderer(), so added
checks in here.

https://codereview.chromium.org/109153003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/109153003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1276: Vector<ImageResource*> images;
Reorganized this function to grab the images out of the style first, and if
there are none then early exit to avoid having to compute the RenderObject's
bounds and grabbing the view rect. This brought the float tests back to normal.

https://codereview.chromium.org/109153003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1278: for (const FillLayer* layer =
blockStyle->backgroundLayers(); layer; layer = layer->next()) {
Swapped the ::access<func> for these (::backgroundLayers() and ::maskLayers())
since the access functions can potentially do copies. This brought the ShadowDom
perf test back to normal.

[eseidel, 2013-12-09 23:57:38]

lgtm

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1278: for (const FillLayer* layer =
blockStyle->backgroundLayers(); layer; layer = layer->next()) {
These two loops look identical. Helper function?

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1311: if (blockStyle->maskBoxImageSource()
&& blockStyle->maskBoxImageSource()->cachedImage())
I might have used a helper here too:

appendIfNotNull(images, blockStyle->maskBoxImageSource())

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1320: // with empty rects. Use
LayoutRect::contains in that case.
What does contains do with an empty rect?  Are you sure this is what you want? 
Should contains just check the point then? Is that what it's doing?

I'm not convinced this if is helping you at all.

Maybe:
bool isVisible = objectBounds.isEmpty() ? viewBounds.intersects(objectBounds) :
true;
?

[shatch, 2013-12-10 01:41:28]

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1278: for (const FillLayer* layer =
blockStyle->backgroundLayers(); layer; layer = layer->next()) {
On 2013/12/09 23:57:39, eseidel wrote:
> These two loops look identical. Helper function?

Done.

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1311: if (blockStyle->maskBoxImageSource()
&& blockStyle->maskBoxImageSource()->cachedImage())
On 2013/12/09 23:57:39, eseidel wrote:
> I might have used a helper here too:
> 
> appendIfNotNull(images, blockStyle->maskBoxImageSource())

Done.

https://codereview.chromium.org/109153003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1320: // with empty rects. Use
LayoutRect::contains in that case.
On 2013/12/09 23:57:39, eseidel wrote:
> What does contains do with an empty rect?  Are you sure this is what you want?

> Should contains just check the point then? Is that what it's doing?
> 
> I'm not convinced this if is helping you at all.
> 
> Maybe:
> bool isVisible = objectBounds.isEmpty() ? viewBounds.intersects(objectBounds)
:
> true;
> ?

Yeah, it's pretty much just checking if the point is in the rect. The idea being
that if the rect's corner is in the view, most likely the image should be higher
priority. Assuming I'm reading that example right (did you mean
!objectBounds.isEmpty() ? ...), I think if we just mark everything with an empty
objectBounds as visible that we'll potentially end up bumping up priority on a
bunch of images that we shouldn't have.

[eseidel, 2013-12-10 17:15:29]

lgtm.  Feel free to ignore the nits.

The only one of substance is possibly refactoring into your helper functions
slightly differnetly.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1286: for (const FillLayer* layer =
styleLayers[i]; layer; layer = layer->next()) {
Oh, I see.  I had envisoned:

appendLayers(images, blockStyle->backgroundLayers());
appendLayers(images, blockStyle->maskLayers());

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1287: if (layer->image()) {
nit: This could now drop at least the inner {}

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1292: ContentData* contentData =
const_cast<ContentData*>(blockStyle->contentData());
The const disease!

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1294: ImageContentData* imageContentData =
static_cast<ImageContentData*>(contentData);
I'm surprised we don't have a toImageContentData() helper yet.  Those seem to be
all the rage in Blink these days.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1298: if (blockStyle->boxReflect()) {
nit: These two ifs could now combine into one line and drop the {}

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1303: if (blockStyle->listStyleImage())
You could even push this check inside if you like.  This is an Image*?

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1318: if (!objectBounds.isEmpty())
I'd just pick the behavior you want for empty rects and short-circut here  Like
I would just say that all empty rects are pretended to be invisible always.  if
(objectBounds.isEmpty()) return;

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:600: bool isVisible;
Same problem.  I wouldn't bother making empty rects visible.  They can't have
any affect on rendering, no?

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2828: for (RenderObject* child =
firstChild(); child; child = child->nextSibling()) {
nit: I wouldn't bother with the {} but that's more personal style.  Blink allows
either.

[eseidel, 2013-12-10 17:17:05]

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1303: if (blockStyle->listStyleImage())
On 2013/12/10 17:15:30, eseidel wrote:
> You could even push this check inside if you like.  This is an Image*?

By this I mean:
appendImageIfNotNull(images, blockSTyle->borderImageSource());
appendImageIfNotNull(images, blockStyle->maskImageSource());
appendImageIfNotNull(images, blockStyle->listStyleImage());

[shatch, 2013-12-10 18:56:19]

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1286: for (const FillLayer* layer =
styleLayers[i]; layer; layer = layer->next()) {
On 2013/12/10 17:15:30, eseidel wrote:
> Oh, I see.  I had envisoned:
> 
> appendLayers(images, blockStyle->backgroundLayers());
> appendLayers(images, blockStyle->maskLayers());

Done.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1287: if (layer->image()) {
On 2013/12/10 17:15:30, eseidel wrote:
> nit: This could now drop at least the inner {}

Done.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1292: ContentData* contentData =
const_cast<ContentData*>(blockStyle->contentData());
On 2013/12/10 17:15:30, eseidel wrote:
> The const disease!

Oops, yeah doesn't even need to be cast.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1298: if (blockStyle->boxReflect()) {
On 2013/12/10 17:15:30, eseidel wrote:
> nit: These two ifs could now combine into one line and drop the {}

Done.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1303: if (blockStyle->listStyleImage())
On 2013/12/10 17:17:05, eseidel wrote:
> On 2013/12/10 17:15:30, eseidel wrote:
> > You could even push this check inside if you like.  This is an Image*?
> 
> By this I mean:
> appendImageIfNotNull(images, blockSTyle->borderImageSource());
> appendImageIfNotNull(images, blockStyle->maskImageSource());
> appendImageIfNotNull(images, blockStyle->listStyleImage());

Done.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:1318: if (!objectBounds.isEmpty())
On 2013/12/10 17:15:30, eseidel wrote:
> I'd just pick the behavior you want for empty rects and short-circut here 
Like
> I would just say that all empty rects are pretended to be invisible always. 
if
> (objectBounds.isEmpty()) return;

Addressed in other comment in RenderImage.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:600: bool isVisible;
On 2013/12/10 17:15:30, eseidel wrote:
> Same problem.  I wouldn't bother making empty rects visible.  They can't have
> any affect on rendering, no?

Per offline discussion: I don't have any data on this either way at the moment,
but I was sort of leaning towards the idea that the rect is empty, but the
position is still somewhat valid, so in that case we'd want to bump up the
priority. Either way, we should investigate more in the near future to figure
out what's best.

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/109153003/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2828: for (RenderObject* child =
firstChild(); child; child = child->nextSibling()) {
On 2013/12/10 17:15:30, eseidel wrote:
> nit: I wouldn't bother with the {} but that's more personal style.  Blink
allows
> either.

Done.

[eseidel, 2013-12-10 19:23:54]

lgtm

[commit-bot: I haz the power, 2013-12-10 19:24:24]

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

[commit-bot: I haz the power, 2013-12-10 20:24:46]

Change committed as 163563