Find cached display items directly during painting

Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983, 510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/95cfc911f623ea4e98e29e74350898fadda14af8
Cr-Commit-Position: refs/heads/master@{#404938}

[Xianzhu, 2016-06-29 16:39:56]

Description was changed from

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
==========

to

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
==========

[Xianzhu, 2016-06-29 16:41:44]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, jbroman@chromium.org, pdr@chromium.org

[Xianzhu, 2016-06-29 16:41:46]

This is still WIP. First cluster telemetry run shows slight regressions (record
time +0.28% ~ +2.3%) that I need to look into.

[Xianzhu, 2016-07-03 00:36:30]

Description was changed from

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
==========

to

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Xianzhu, 2016-07-06 17:27:56]

On 2016/06/29 16:41:46, Xianzhu wrote:
> This is still WIP. First cluster telemetry run shows slight regressions
(record
> time +0.28% ~ +2.3%) that I need to look into.

Based on the results of multiple ct runs, the regression of
record_time_subsequence_caching_disabled and record_time_caching_disabled is
about 1%. This may be caused by the variables that were accessed as local
variables in commitNewDisplayItems now changed to class members of the
PaintController. I think this is acceptable given that this CL resolves the
problem of subsequence caching in spv2 and others.

Ptal.

[jbroman, 2016-07-06 17:34:03]

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp
(right):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp:176: while
(m_endIndex < m_buffers.size() - 1)
How are we getting into a state where this is needed?
ContiguousContainerBase::removeLast should already be removing all but one empty
buffer.

[Xianzhu, 2016-07-06 18:05:27]

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp
(right):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp:176: while
(m_endIndex < m_buffers.size() - 1)
On 2016/07/06 17:34:03, jbroman wrote:
> How are we getting into a state where this is needed?
> ContiguousContainerBase::removeLast should already be removing all but one
empty
> buffer.

The method is to remove the last empty buffer. For a committed display item
list, because we will no more change the list, the empty buffer (which is big)
will be never used.

This was not needed because we copied the items in m_newDisplayItemList into
updatedList which doesn't have removeLast() calls so doesn't have the trailing
empty buffer, and then move updatedList into m_currentPaintArtifact. Now we move
m_newDisplayItemList into m_currentPaintArtifact directly. If there is any
trailing empty buffer it will be alive until the next paint.

[jbroman, 2016-07-06 19:09:38]

I like this patch in principle.

Bot failures look real; can you resolve them?

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp
(right):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.cpp:176: while
(m_endIndex < m_buffers.size() - 1)
On 2016/07/06 at 18:05:27, Xianzhu wrote:
> On 2016/07/06 17:34:03, jbroman wrote:
> > How are we getting into a state where this is needed?
> > ContiguousContainerBase::removeLast should already be removing all but one
empty
> > buffer.
> 
> The method is to remove the last empty buffer. For a committed display item
list, because we will no more change the list, the empty buffer (which is big)
will be never used.
> 
> This was not needed because we copied the items in m_newDisplayItemList into
updatedList which doesn't have removeLast() calls so doesn't have the trailing
empty buffer, and then move updatedList into m_currentPaintArtifact. Now we move
m_newDisplayItemList into m_currentPaintArtifact directly. If there is any
trailing empty buffer it will be alive until the next paint.

Ah, okay, it's a "shrink to fit"-ish thing. Makes sense.

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:62: void
removeEmptyBuffers();
Mind a comment clarifying the intended use of this? I'm a little worried it will
imply that we otherwise never discard buffers. Something like:

// Discards excess buffer capacity. Intended for use when no more appending is
anticipated.

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp
(left):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp:19: // No
reason to continue the analysis once we have a veto.
Why is this no longer needed?

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:207: void
resetCurrentListIterators();
Comment? It's not obvious to me what the "current list iterators" are or why one
might wish to reset them.

[Xianzhu, 2016-07-06 19:29:34]

The failures seem caused by misaligned TransformationMatrix. 

Rebased the CL on https://codereview.chromium.org/2122573002/ to see if the
failures still happen.

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:62: void
removeEmptyBuffers();
On 2016/07/06 19:09:37, jbroman wrote:
> Mind a comment clarifying the intended use of this? I'm a little worried it
will
> imply that we otherwise never discard buffers. Something like:
> 
> // Discards excess buffer capacity. Intended for use when no more appending is
> anticipated.

Done.

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp
(left):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItemList.cpp:19: // No
reason to continue the analysis once we have a veto.
On 2016/07/06 19:09:37, jbroman wrote:
> Why is this no longer needed?

Now this is done in PaintController::commitNewDisplayItems().

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2107103002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:207: void
resetCurrentListIterators();
On 2016/07/06 19:09:38, jbroman wrote:
> Comment? It's not obvious to me what the "current list iterators" are or why
one
> might wish to reset them.

Done.

[Xianzhu, 2016-07-06 22:31:37]

There are still failures of several spv2 tests. The new method is still not
fully working for spv2. Need to change copyCachedSubsequence() to handle cached
chunks and paint properties which seems not trivial.

Discovered this until just now because I didn't enable subsequence caching for
spv2 for earlier patch sets.

Investigating this and will upload a new patch soon.

[Xianzhu, 2016-07-06 22:32:26]

Description was changed from

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Xianzhu, 2016-07-11 19:05:58]

On 2016/07/06 22:31:37, Xianzhu wrote:
> There are still failures of several spv2 tests. The new method is still not
> fully working for spv2. Need to change copyCachedSubsequence() to handle
cached
> chunks and paint properties which seems not trivial.
> 
> Discovered this until just now because I didn't enable subsequence caching for
> spv2 for earlier patch sets.
> 
> Investigating this and will upload a new patch soon.

The situation looks more complicated than I expected. When a cached subsequence
is used, we need to recover not only the chunks in it, but also the chunk paint
properties. This needs more investigation.

Though the main purpose of this CL is for subsequence caching support for spv2,
it can't reach the goal yet, so this CL is a refactory on spv1 towards the spv2
goal.

Ptal.

[pdr., 2016-07-12 02:19:52]

Really nice! This cleans up a lot of complexity.


You said "Though the main purpose of this CL is for subsequence caching support
for spv2, it hasn't reached the goal yet, so this CL is a refactoring of spv1
towards the spv2 goal.", do you mean that a followup patch will fix some loose
ends? If so, can you add TODOs? It isn't clear to me which parts are broken.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:65: void
removeEmptyBuffers();
ContiguousContainer's buffers are sort of an internal implementation detail. Can
we avoid exposing this publicly? For example, we could rename this
"shrinkToFit()" or "compactUnusedMemory()"?

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:371:
m_newDisplayItemList.removeEmptyBuffers();
Can you add a comment here, like:
// The new list will not be appended to again so we can release unused memory.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:377:
m_newDisplayItemList = DisplayItemList(0);
Is the "m_newDisplayItemList.removeEmptyBuffers();" call above needed if we just
re-create the list here?

Do you know if the approach here is needed for performance reasons? I'm unsure
if it is worth it to lazily allocate the initial buffer... it seems more
expensive to have an empty check on every append call than it would to just
pre-allocate kInitialDisplayItemListCapacityBytes immediately. By keeping around
the same initial buffer, we can also avoid allocations.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:426: #if 0
// DCHECK_IS_ON()
Did you plan on fixing this in this patch?

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:169: ,
m_numSequentialMatches(0)
This patch adds a lot of new state (7 members) and complexity that's specific to
cache lookups. Do you think it is possible to package that up and hide it in a
class/struct?

Maybe something like "class CachedDisplayItemLookup" with a lifetime that's
restricted to the paint phase (i.e., the dtor resets the list iterators, etc)?

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp:16:
return context.getPaintController().useCachedSubsequenceIfPossible(client);
Would this inline if it's in the header?

[Xianzhu, 2016-07-12 17:56:48]

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:65: void
removeEmptyBuffers();
On 2016/07/12 02:19:51, pdr. wrote:
> ContiguousContainer's buffers are sort of an internal implementation detail.
Can
> we avoid exposing this publicly? For example, we could rename this
> "shrinkToFit()" or "compactUnusedMemory()"?

Done.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:371:
m_newDisplayItemList.removeEmptyBuffers();
On 2016/07/12 02:19:51, pdr. wrote:
> Can you add a comment here, like:
> // The new list will not be appended to again so we can release unused memory.

Done.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:377:
m_newDisplayItemList = DisplayItemList(0);
On 2016/07/12 02:19:52, pdr. wrote:
> Is the "m_newDisplayItemList.removeEmptyBuffers();" call above needed if we
just
> re-create the list here?

The removeEmptyBuffers() is for the m_currentPaintArtifact which holds the
m_newDisplayItemList before line 372.

Actually the above statement is not necessary but may be clearer about the
intent.

> 
> Do you know if the approach here is needed for performance reasons? I'm unsure
> if it is worth it to lazily allocate the initial buffer... it seems more
> expensive to have an empty check on every append call than it would to just
> pre-allocate kInitialDisplayItemListCapacityBytes immediately. By keeping
around
> the same initial buffer, we can also avoid allocations.

This is a balance between memory and cpu performance. If we allocate the initial
buffer now, we'll keep the buffer during the time between paint phases which may
have much longer duration than the paint phases, e.g. when the page is idle.

However, this will be not a problem when we make PaintController live in a paint
phase only
(https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...).

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:426: #if 0
// DCHECK_IS_ON()
On 2016/07/12 02:19:51, pdr. wrote:
> Did you plan on fixing this in this patch?

https://codereview.chromium.org/2114653003/ based on this patch will fix this.

We can either review and land them one by one, review them one by one but land
them together, or review and land them as one. Which do you prefer?

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:169: ,
m_numSequentialMatches(0)
On 2016/07/12 02:19:52, pdr. wrote:
> This patch adds a lot of new state (7 members) and complexity that's specific
to
> cache lookups. Do you think it is possible to package that up and hide it in a
> class/struct?
> 
> Maybe something like "class CachedDisplayItemLookup" with a lifetime that's
> restricted to the paint phase (i.e., the dtor resets the list iterators, etc)?

Actually most of the fields and methods of PaintController are for paint phase
only, not just the cache lookup ones, so I'd like to make the whole
PaintController live in paint phase only, like the following:

   GraphicsLayer /* or some other class */ {
     PaintArtifact m_currentPaintArtifact;
   }

   GraphicsLayer::paint() {
      PaintController paintController(m_currentPaintArtifact);
      GraphicsContext context(paintController);
      paint the contents;
      m_currentPaintArtifact = paintController.commit();
   }

So for this CL I'd like to keep the fields as-is.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/SubsequenceRecorder.cpp:16:
return context.getPaintController().useCachedSubsequenceIfPossible(client);
On 2016/07/12 02:19:52, pdr. wrote:
> Would this inline if it's in the header?

Done.

[jbroman, 2016-07-12 19:05:50]

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:169: ,
m_numSequentialMatches(0)
On 2016/07/12 at 17:56:47, Xianzhu wrote:
> On 2016/07/12 02:19:52, pdr. wrote:
> > This patch adds a lot of new state (7 members) and complexity that's
specific to
> > cache lookups. Do you think it is possible to package that up and hide it in
a
> > class/struct?
> > 
> > Maybe something like "class CachedDisplayItemLookup" with a lifetime that's
> > restricted to the paint phase (i.e., the dtor resets the list iterators,
etc)?
> 
> Actually most of the fields and methods of PaintController are for paint phase
only, not just the cache lookup ones, so I'd like to make the whole
PaintController live in paint phase only, like the following:
> 
>    GraphicsLayer /* or some other class */ {
>      PaintArtifact m_currentPaintArtifact;
>    }
> 
>    GraphicsLayer::paint() {
>       PaintController paintController(m_currentPaintArtifact);
>       GraphicsContext context(paintController);
>       paint the contents;
>       m_currentPaintArtifact = paintController.commit();
>    }
> 
> So for this CL I'd like to keep the fields as-is.

I'd assumed there might be information we wish to store from frame to frame
aside from the artifact itself. If not, making PaintController ephemeral sounds
reasonable.

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:63: //
Discards excess bufer capacity. Intended for use when no more appending
nit: bufer -> buffer

[pdr., 2016-07-12 20:09:36]

Looks great, just a couple of questions before l-g-t-m.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:426: #if 0
// DCHECK_IS_ON()
On 2016/07/12 at 17:56:47, Xianzhu wrote:
> On 2016/07/12 02:19:51, pdr. wrote:
> > Did you plan on fixing this in this patch?
> 
> https://codereview.chromium.org/2114653003/ based on this patch will fix this.
> 
> We can either review and land them one by one, review them one by one but land
them together, or review and land them as one. Which do you prefer?

Separate patches sounds good to me.

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:169: ,
m_numSequentialMatches(0)
On 2016/07/12 at 17:56:47, Xianzhu wrote:
> On 2016/07/12 02:19:52, pdr. wrote:
> > This patch adds a lot of new state (7 members) and complexity that's
specific to
> > cache lookups. Do you think it is possible to package that up and hide it in
a
> > class/struct?
> > 
> > Maybe something like "class CachedDisplayItemLookup" with a lifetime that's
> > restricted to the paint phase (i.e., the dtor resets the list iterators,
etc)?
> 
> Actually most of the fields and methods of PaintController are for paint phase
only, not just the cache lookup ones, so I'd like to make the whole
PaintController live in paint phase only, like the following:
> 
>    GraphicsLayer /* or some other class */ {
>      PaintArtifact m_currentPaintArtifact;
>    }
> 
>    GraphicsLayer::paint() {
>       PaintController paintController(m_currentPaintArtifact);
>       GraphicsContext context(paintController);
>       paint the contents;
>       m_currentPaintArtifact = paintController.commit();
>    }
This plan sounds good to me.

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:51:
DisplayItemList::iterator it = findCachedItem(DisplayItem::Id(client, type));
Super nit: "it" -> "cachedItemIterator" or "cachedItem"

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:91:
DisplayItemList::iterator it = findCachedItem(DisplayItem::Id(client,
DisplayItem::Subsequence));
Super nit: "it" -> "firstCachedItemIterator" or "firstCachedItem"

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:97: //
|it| will point to the first item after the subsequence or end of the current
list.
Before copyCachedSubsequence, won't |it| point to the first item of the
subsequence, not the first item after it?

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:427:
resetCurrentListIterators();
Why is this needed?

[Xianzhu, 2016-07-13 00:28:22]

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2107103002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:169: ,
m_numSequentialMatches(0)
On 2016/07/12 19:05:49, jbroman wrote:
> On 2016/07/12 at 17:56:47, Xianzhu wrote:
> > On 2016/07/12 02:19:52, pdr. wrote:
> > > This patch adds a lot of new state (7 members) and complexity that's
> specific to
> > > cache lookups. Do you think it is possible to package that up and hide it
in
> a
> > > class/struct?
> > > 
> > > Maybe something like "class CachedDisplayItemLookup" with a lifetime
that's
> > > restricted to the paint phase (i.e., the dtor resets the list iterators,
> etc)?
> > 
> > Actually most of the fields and methods of PaintController are for paint
phase
> only, not just the cache lookup ones, so I'd like to make the whole
> PaintController live in paint phase only, like the following:
> > 
> >    GraphicsLayer /* or some other class */ {
> >      PaintArtifact m_currentPaintArtifact;
> >    }
> > 
> >    GraphicsLayer::paint() {
> >       PaintController paintController(m_currentPaintArtifact);
> >       GraphicsContext context(paintController);
> >       paint the contents;
> >       m_currentPaintArtifact = paintController.commit();
> >    }
> > 
> > So for this CL I'd like to keep the fields as-is.
> 
> I'd assumed there might be information we wish to store from frame to frame
> aside from the artifact itself. If not, making PaintController ephemeral
sounds
> reasonable.

We need to store m_currentCacheGeneration besides m_currentPaintArtifact
crossing frames. We can make m_currentCacheGeneration a field of PaintArtifact.
m_textPainted and m_imagePainted can be stored in GraphicsLayer. Other fields
can all live within a paint phase.

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContiguousContainer.h (right):

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContiguousContainer.h:63: //
Discards excess bufer capacity. Intended for use when no more appending
On 2016/07/12 19:05:49, jbroman wrote:
> nit: bufer -> buffer

Done.

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:51:
DisplayItemList::iterator it = findCachedItem(DisplayItem::Id(client, type));
On 2016/07/12 20:09:35, pdr. wrote:
> Super nit: "it" -> "cachedItemIterator" or "cachedItem"

Done.

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:91:
DisplayItemList::iterator it = findCachedItem(DisplayItem::Id(client,
DisplayItem::Subsequence));
On 2016/07/12 20:09:35, pdr. wrote:
> Super nit: "it" -> "firstCachedItemIterator" or "firstCachedItem"

Renamed to cachedItem instead of firstCachedItem because later it will point to
later items.

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:97: //
|it| will point to the first item after the subsequence or end of the current
list.
On 2016/07/12 20:09:35, pdr. wrote:
> Before copyCachedSubsequence, won't |it| point to the first item of the
> subsequence, not the first item after it?

Before copyCachedSubsequence |it| points to the first item of the subsequence.
It looks natural that during copyCachedSubsequence it iterates all items in the
subsequence and results after the subsequence.

https://codereview.chromium.org/2107103002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:427:
resetCurrentListIterators();
On 2016/07/12 20:09:35, pdr. wrote:
> Why is this needed?

We need to reset the iterators after mutation of the DisplayItemList which may
cause the original iterators to be invalid (pointing to deallocated memory
location). Added a comment here.

[pdr., 2016-07-13 00:32:46]

LGTM

[Xianzhu, 2016-07-13 00:50:35]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-07-13 00:53:17]

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-13 03:00:36]

Description was changed from

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-07-13 03:00:38]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-07-13 03:01:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 03:02:02]

Description was changed from

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Find cached display items directly during painting

Instead of adding CachedDisplayItems in the new display item list and
replace them with real cached display items during commit, now find and
add the real cached display items during painting directly.

The main purpose is to let PaintChunker work for cached subsequences.

Also has other benefits:
- Avoid another display list during commit;
- Avoid creating CachedDisplayItems.

BUG=596983,510908
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/95cfc911f623ea4e98e29e74350898fadda14af8
Cr-Commit-Position: refs/heads/master@{#404938}
==========

[commit-bot: I haz the power, 2016-07-13 03:02:04]

Patchset 18 (id:??) landed as
https://crrev.com/95cfc911f623ea4e98e29e74350898fadda14af8
Cr-Commit-Position: refs/heads/master@{#404938}