Avoid re-iterate out-of-order display items

Avoid re-iterate out-of-order display items

Previously for the following case:
 current display items:
    A B C D
 new display items:
    C B A D

A and B in current display items will be skipped and indexed when adding
C, then the index will be used when adding B and A.

When adding D, currentIt points to A, and B and C will be iterated
again.

Add nextItemToIndexIt variable to memorize the place to index, to avoid
repeated iteration.

BTW
- Change DisplayItem::ignoreInDisplayList() to (!)DisplayItem::isValid()
- Let currentIt point to the next item of the item just being copied so
  that we can synchronize sooner in some cases.

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

[Xianzhu, 2015-08-17 23:06:32]

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

[pdr., 2015-08-17 23:28:50]

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:332: void invalidate() { new (this)
DisplayItem; }
Will this run the DisplayItem subclass destructors?

The in-place new constructor is rarely used in blink. Can you add a comment such
as:
// Re-construct this display item as a permanently invalid item with no client
set.

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:307: ASSERT(foundIt !=
currentIt); // otherwise isSynchronized should be true.
ASSERT(isSynchronized)?

[Xianzhu, 2015-08-17 23:46:59]

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:332: void invalidate() { new (this)
DisplayItem; }
On 2015/08/17 23:28:50, pdr wrote:
> Will this run the DisplayItem subclass destructors?
> 

No but needed. Added an in-place destructor call.

> The in-place new constructor is rarely used in blink. Can you add a comment
such
> as:
> // Re-construct this display item as a permanently invalid item with no client
> set.

Done.

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1287863003/diff/10005/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:307: ASSERT(foundIt !=
currentIt); // otherwise isSynchronized should be true.
On 2015/08/17 23:28:50, pdr wrote:
> ASSERT(isSynchronized)?

The comment above seems confusing.

Changed to:
    ASSERT(foundIt != currentIt); // because we are in 'if (!isSynchronized)'

[pdr., 2015-08-18 06:15:21]

This is a cool optimization. LGTM with just a few minor fixes.

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:333: void invalidate()
This looks correct now, but it's tricky. Because this codepath is only used for
underinvalidation checking, what do you think about renaming invalidate() to
clearClientForUnderinvalidationChecking(), changing m_client to be mutable, and
directly setting m_client to nullptr?

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:191: return
nextItemToIndexIt++;
Clever :)

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:276: DisplayItems::iterator
nextItemToIndexIt = currentIt;
Can you move this up below displayItemIndicesByClient to help make it clear that
they are related?

Bikeshed: We've tried to keep the details of the out-of-order optimization out
of the main merge loop for simplicity. It may even make sense to wrap
displayItemIndicesByClient and nextItemToIndex together in a struct, but lets
save that for a followup. For now, what do you think about renaming
nextItemToIndexIt to nextItemToIndexByClient?

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:289: // Skip indexing of
copied items.
I think it will be best to hide this detail inside findOutOfOrderCachedItem. Can
you change findOutOfOrderCachedItem to take (currentIt, newDisplayItemId,
displayItemIndicesByClient, nextItemToIndexIt) and then have
findOutOfOrderCachedItem update nextItemToIndexIt?

[Xianzhu, 2015-08-18 16:24:44]

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItem.h (right):

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItem.h:333: void invalidate()
On 2015/08/18 06:15:21, pdr wrote:
> This looks correct now, but it's tricky. Because this codepath is only used
for
> underinvalidation checking, what do you think about renaming invalidate() to
> clearClientForUnderinvalidationChecking(), changing m_client to be mutable,
and
> directly setting m_client to nullptr?

Acknowledged and Done.

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemList.cpp (right):

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:276: DisplayItems::iterator
nextItemToIndexIt = currentIt;
On 2015/08/18 06:15:21, pdr wrote:
> Can you move this up below displayItemIndicesByClient to help make it clear
that
> they are related?
> 
> Bikeshed: We've tried to keep the details of the out-of-order optimization out
> of the main merge loop for simplicity. It may even make sense to wrap
> displayItemIndicesByClient and nextItemToIndex together in a struct, but lets
> save that for a followup. For now, what do you think about renaming
> nextItemToIndexIt to nextItemToIndexByClient?

Done.

https://codereview.chromium.org/1287863003/diff/30001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemList.cpp:289: // Skip indexing of
copied items.
On 2015/08/18 06:15:21, pdr wrote:
> I think it will be best to hide this detail inside findOutOfOrderCachedItem.
Can
> you change findOutOfOrderCachedItem to take (currentIt, newDisplayItemId,
> displayItemIndicesByClient, nextItemToIndexIt) and then have
> findOutOfOrderCachedItem update nextItemToIndexIt?

Done.

[Xianzhu, 2015-08-18 16:24:50]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-08-18 16:24:50]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1287863003/#ps50001
(title: "Address pdr's comments")

[commit-bot: I haz the power, 2015-08-18 16:25:00]

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

[commit-bot: I haz the power, 2015-08-18 16:35:04]

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

[commit-bot: I haz the power, 2015-08-18 16:35:05]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Xianzhu, 2015-08-18 16:39:06]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-08-18 16:39:06]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1287863003/#ps70001
(title: " ")

[commit-bot: I haz the power, 2015-08-18 16:39:12]

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

[commit-bot: I haz the power, 2015-08-18 18:16:22]

Committed patchset #5 (id:70001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200736