Refactor ResourceLoadPriorityOptimizer to avoid walking render tree

Refactor ResourceLoadPriorityOptimizer to avoid walking render tree after every layout and scroll event.

1. Instead of calling RenderObject::didScroll on every
FrameView::scrollPositionChanged, use a 250 ms timer to update the
image priorities. Ideally, we only update the image priorities once
now about 250 ms after scrolling stops.

2. Whenever a RenderImage/RenderBlock is created, it's added to the
ResourceLoadPriorityOptimizer. After a layout/scroll, iterate the list
removing any RenderObject that either has no images, or has loaded all
of it's images.


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

[esprehn, 2014-01-13 23:29:42]

Most things in a page are RenderBlocks, which means this HashSet will usually be
about the same size as the render tree itself. Instead I'd suggest adding a
bitfield to RenderObject for "has resources" and "child has resources" and then
walk the tree following those bits just like in ::layout() or ::recalcStyle().

The alternative is to add yourself to the hash set when you're first created
only if you have images, which means you want to hook ::styleDidChange to add
yourself to the hash set. That works too.

[shatch, 2014-01-14 00:37:19]

On 2014/01/13 23:29:42, esprehn wrote:
> Most things in a page are RenderBlocks, which means this HashSet will usually
be
> about the same size as the render tree itself. Instead I'd suggest adding a
> bitfield to RenderObject for "has resources" and "child has resources" and
then
> walk the tree following those bits just like in ::layout() or ::recalcStyle().
> 
> The alternative is to add yourself to the hash set when you're first created
> only if you have images, which means you want to hook ::styleDidChange to add
> yourself to the hash set. That works too.

Ok, went with 2nd suggestion and hooked into RenderBlock::styleDidChange and
checked for images there.

ptal

[esprehn, 2014-01-15 01:27:44]

This looks promising.

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:63: void
ResourceLoadPriorityOptimizer::addRenderObject(RenderObject* o)
object or renderer, no single letter vars please. :)

https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:70: m_objectCache.remove(o);
m_objects or something, I'd not use objectCache, that means a lot of different
things in blink.

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:74: friend
ResourceLoadPriorityOptimizer* resourceLoadPriorityOptimizer();
Can this just be a static thing instead? Why does it need to be a friend?

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:115: static const double
timeToWaitAfterScrollInterval = 0.250;
Needs a better name, resourcePriorityUpdateDelayAfterScroll maybe?

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:199: static void
appendImagesFromStyle(Vector<ImageResource*>& images, RenderStyle* blockStyle)
RenerStyle& blockStyle

https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:356:
resourceLoadPriorityOptimizer()->removeRenderObject(this);
This isn't good, you're going to hit the hash table in the remove() call all the
time, usually we use a bit to avoid this. I'd add a bit for tracking if this
renderer is actually in that hash table.

addRenderObject does renderer->setHasPendingResourceUpdate() or something like
that.
removeRenderObject does:
if (!renderer->hasPendingResourceUpdate())
  return;
m_objects.remove(renderer);
renderer->setHasPendingResourcesUpdate(false);

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:78:
resourceLoadPriorityOptimizer()->removeRenderObject(this);
This should be in ~RenderObject and you should just check that bitfield. That
way we can never forget a renderer.

[shatch, 2014-01-15 19:28:51]

New snapshot uploaded, ptal.

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:63: void
ResourceLoadPriorityOptimizer::addRenderObject(RenderObject* o)
On 2014/01/15 01:27:45, esprehn wrote:
> object or renderer, no single letter vars please. :)

Done.

https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:70: m_objectCache.remove(o);
On 2014/01/15 01:27:45, esprehn wrote:
> m_objects or something, I'd not use objectCache, that means a lot of different
> things in blink.

Done.

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:74: friend
ResourceLoadPriorityOptimizer* resourceLoadPriorityOptimizer();
On 2014/01/15 01:27:45, esprehn wrote:
> Can this just be a static thing instead? Why does it need to be a friend?

Done.

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:115: static const double
timeToWaitAfterScrollInterval = 0.250;
On 2014/01/15 01:27:45, esprehn wrote:
> Needs a better name, resourcePriorityUpdateDelayAfterScroll maybe?

Done.

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:199: static void
appendImagesFromStyle(Vector<ImageResource*>& images, RenderStyle* blockStyle)
On 2014/01/15 01:27:45, esprehn wrote:
> RenerStyle& blockStyle

Done.

https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:356:
resourceLoadPriorityOptimizer()->removeRenderObject(this);
On 2014/01/15 01:27:45, esprehn wrote:
> This isn't good, you're going to hit the hash table in the remove() call all
the
> time, usually we use a bit to avoid this. I'd add a bit for tracking if this
> renderer is actually in that hash table.
> 
> addRenderObject does renderer->setHasPendingResourceUpdate() or something like
> that.
> removeRenderObject does:
> if (!renderer->hasPendingResourceUpdate())
>   return;
> m_objects.remove(renderer);
> renderer->setHasPendingResourcesUpdate(false);

Done.

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

https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderImage.cpp:78:
resourceLoadPriorityOptimizer()->removeRenderObject(this);
On 2014/01/15 01:27:45, esprehn wrote:
> This should be in ~RenderObject and you should just check that bitfield. That
> way we can never forget a renderer.

Done.

[shatch, 2014-01-22 00:49:18]

friendly ping

On 2014/01/15 19:28:51, shatch wrote:
> New snapshot uploaded, ptal.
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
> File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
> Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:63: void
> ResourceLoadPriorityOptimizer::addRenderObject(RenderObject* o)
> On 2014/01/15 01:27:45, esprehn wrote:
> > object or renderer, no single letter vars please. :)
> 
> Done.
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
> Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:70:
m_objectCache.remove(o);
> On 2014/01/15 01:27:45, esprehn wrote:
> > m_objects or something, I'd not use objectCache, that means a lot of
different
> > things in blink.
> 
> Done.
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
> File Source/core/fetch/ResourceLoadPriorityOptimizer.h (right):
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/fetch/Resour...
> Source/core/fetch/ResourceLoadPriorityOptimizer.h:74: friend
> ResourceLoadPriorityOptimizer* resourceLoadPriorityOptimizer();
> On 2014/01/15 01:27:45, esprehn wrote:
> > Can this just be a static thing instead? Why does it need to be a friend?
> 
> Done.
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/frame/FrameV...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/frame/FrameV...
> Source/core/frame/FrameView.cpp:115: static const double
> timeToWaitAfterScrollInterval = 0.250;
> On 2014/01/15 01:27:45, esprehn wrote:
> > Needs a better name, resourcePriorityUpdateDelayAfterScroll maybe?
> 
> Done.
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderBlock.cpp:199: static void
> appendImagesFromStyle(Vector<ImageResource*>& images, RenderStyle* blockStyle)
> On 2014/01/15 01:27:45, esprehn wrote:
> > RenerStyle& blockStyle
> 
> Done.
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderBlock.cpp:356:
> resourceLoadPriorityOptimizer()->removeRenderObject(this);
> On 2014/01/15 01:27:45, esprehn wrote:
> > This isn't good, you're going to hit the hash table in the remove() call all
> the
> > time, usually we use a bit to avoid this. I'd add a bit for tracking if this
> > renderer is actually in that hash table.
> > 
> > addRenderObject does renderer->setHasPendingResourceUpdate() or something
like
> > that.
> > removeRenderObject does:
> > if (!renderer->hasPendingResourceUpdate())
> >   return;
> > m_objects.remove(renderer);
> > renderer->setHasPendingResourcesUpdate(false);
> 
> Done.
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
> File Source/core/rendering/RenderImage.cpp (right):
> 
>
https://codereview.chromium.org/131233003/diff/40001/Source/core/rendering/Re...
> Source/core/rendering/RenderImage.cpp:78:
> resourceLoadPriorityOptimizer()->removeRenderObject(this);
> On 2014/01/15 01:27:45, esprehn wrote:
> > This should be in ~RenderObject and you should just check that bitfield.
That
> > way we can never forget a renderer.
> 
> Done.

[esprehn, 2014-01-22 01:06:43]

lgtm, the timer makes me a little nervous since the behavior is less
predictable, but we can see where it takes us.

[commit-bot: I haz the power, 2014-01-22 20:45:23]

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

[commit-bot: I haz the power, 2014-01-23 00:56:50]

Change committed as 165582

[ojan, 2014-01-23 06:32:59]

Some drive-by questions...

https://codereview.chromium.org/131233003/diff/140002/Source/core/fetch/Resou...
File Source/core/fetch/ResourceLoadPriorityOptimizer.h (right):

https://codereview.chromium.org/131233003/diff/140002/Source/core/fetch/Resou...
Source/core/fetch/ResourceLoadPriorityOptimizer.h:73: typedef
HashSet<RenderObject*> RenderObjectSet;
Since we only ever add RenderImages, could we tighten up this type?

https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:218:
ResourceLoadPriorityOptimizer::resourceLoadPriorityOptimizer()->removeRenderObject(this);
Is this needed with the RenderObject destructor also calling this?

https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
Source/core/rendering/RenderObject.cpp:239:
ResourceLoadPriorityOptimizer::resourceLoadPriorityOptimizer()->removeRenderObject(this);
Since we only ever add RenderImages, could we just call this from the
RenderImage destructor?

[shatch, 2014-01-27 17:47:47]

Sorry ojan, didn't see this message!

Replies below:

On 2014/01/23 06:32:59, ojan wrote:
> Some drive-by questions...
> 
>
https://codereview.chromium.org/131233003/diff/140002/Source/core/fetch/Resou...
> File Source/core/fetch/ResourceLoadPriorityOptimizer.h (right):
> 
>
https://codereview.chromium.org/131233003/diff/140002/Source/core/fetch/Resou...
> Source/core/fetch/ResourceLoadPriorityOptimizer.h:73: typedef
> HashSet<RenderObject*> RenderObjectSet;
> Since we only ever add RenderImages, could we tighten up this type?
> 

We add RenderBlocks as well.

>
https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
> Source/core/rendering/RenderBlock.cpp:218:
>
ResourceLoadPriorityOptimizer::resourceLoadPriorityOptimizer()->removeRenderObject(this);
> Is this needed with the RenderObject destructor also calling this?
> 

You're right, I'll remove this and upload a CL.

>
https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
> File Source/core/rendering/RenderObject.cpp (right):
> 
>
https://codereview.chromium.org/131233003/diff/140002/Source/core/rendering/R...
> Source/core/rendering/RenderObject.cpp:239:
>
ResourceLoadPriorityOptimizer::resourceLoadPriorityOptimizer()->removeRenderObject(this);
> Since we only ever add RenderImages, could we just call this from the
> RenderImage destructor?

We add RenderBlocks as well.