Disable drawing recorder cache for carets

Disable drawing recorder cache for carets

Carets paint into other renderer's display items and can change
without invalidating! This leads to problems in the display list
update because cached display items should not change. This patch
adds a cache disabler for caret rendering.

Carrot rendering is unaffected by this change: http://pr.gg/carrot_display_list.png

BUG=441108

[pdr., 2014-12-11 20:21:11]

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

[Xianzhu, 2014-12-11 20:50:04]

lgtm.

It's great that slimming paint can also reduce painting for caret. Previously I
thought of letting caret be in a composited layer to reduce painting of the
contents under the caret.

I'd like to still make clear of more details about caret invalidation and
painting:

- It seems not to go through RenderObject::invalidatePaintUsingContainer(),
otherwise we should have invalidated the renderer in the displayItemList.

- When the caret is to be erased, in old painting mode we need to paint the
contents under the caret but don't need to paint the caret itself. How do we
erase a caret in slimming paint mode?

I could look into them.

[pdr., 2014-12-11 21:01:08]

On 2014/12/11 at 20:50:04, wangxianzhu wrote:
> lgtm.
> 
> It's great that slimming paint can also reduce painting for caret. Previously
I thought of letting caret be in a composited layer to reduce painting of the
contents under the caret.
> 
> I'd like to still make clear of more details about caret invalidation and
painting:
> 
> - It seems not to go through RenderObject::invalidatePaintUsingContainer(),
otherwise we should have invalidated the renderer in the displayItemList.
> 
> - When the caret is to be erased, in old painting mode we need to paint the
contents under the caret but don't need to paint the caret itself. How do we
erase a caret in slimming paint mode?
> 
> I could look into them.

These are good questions.

You're correct that this does not go through invalidatePaintUsingContainer. I
suspect it's that way because invalidatePaintUsingContainer would cause the body
to be constantly invalidated on many pages.

The caret display items should be removed on every update because they aren't
cached (the clientCacheIsValid check). We might want to add a special type for
carets to make this easier to debug, what do you think about changing the
display item type to be PaintPhaseForegroundCaret?

In a slimming paint world we'll have display items for the content under the
caret because caret drawing is "overdrawn", so I think this will just work.

[Xianzhu, 2014-12-11 21:46:41]

(Got 'Internal error' when replying in the CL, so reply the email here)

Just noticed this and am wondering if it's a real issue: when the caret is
to be erased, the DrawingRecorder will generate an empty picture for the
caret and won't emit a new display item. It seems that we need to
explicitly invalidate the original display item which is for the visible
caret, otherwise the display item will be kept. However, for now we can
only invalidate by renderer. Perhaps it's time to support per-display-item
invalidation?

On Thu, Dec 11, 2014 at 1:01 PM, <pdr@chromium.org> wrote:
>
> On 2014/12/11 at 20:50:04, wangxianzhu wrote:
>
>> lgtm.
>>
>
>  It's great that slimming paint can also reduce painting for caret.
>> Previously
>>
> I thought of letting caret be in a composited layer to reduce painting of
> the
> contents under the caret.
>
>  I'd like to still make clear of more details about caret invalidation and
>>
> painting:
>
>  - It seems not to go through RenderObject::invalidatePaintUsingContainer(
>> ),
>>
> otherwise we should have invalidated the renderer in the displayItemList.
>
>  - When the caret is to be erased, in old painting mode we need to paint
>> the
>>
> contents under the caret but don't need to paint the caret itself. How do
> we
> erase a caret in slimming paint mode?
>
>  I could look into them.
>>
>
> These are good questions.
>
> You're correct that this does not go through
> invalidatePaintUsingContainer. I
> suspect it's that way because invalidatePaintUsingContainer would cause
> the body
> to be constantly invalidated on many pages.
>
> The caret display items should be removed on every update because they
> aren't
> cached (the clientCacheIsValid check). We might want to add a special type
> for
> carets to make this easier to debug, what do you think about changing the
> display item type to be PaintPhaseForegroundCaret?
>
> In a slimming paint world we'll have display items for the content under
> the
> caret because caret drawing is "overdrawn", so I think this will just work.
>
> https://codereview.chromium.org/800553002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chrishtr, 2014-12-11 21:48:36]

I also got an error. My comment:

Also, how exactly do carets paint without invalidation?

How much work is it for carets to start invalidating their containing block?

On Thu, Dec 11, 2014 at 1:46 PM, Xianzhu Wang <wangxianzhu@chromium.org>
wrote:

> (Got 'Internal error' when replying in the CL, so reply the email here)
>
> Just noticed this and am wondering if it's a real issue: when the caret is
> to be erased, the DrawingRecorder will generate an empty picture for the
> caret and won't emit a new display item. It seems that we need to
> explicitly invalidate the original display item which is for the visible
> caret, otherwise the display item will be kept. However, for now we can
> only invalidate by renderer. Perhaps it's time to support per-display-item
> invalidation?
>
> On Thu, Dec 11, 2014 at 1:01 PM, <pdr@chromium.org> wrote:
>>
>> On 2014/12/11 at 20:50:04, wangxianzhu wrote:
>>
>>> lgtm.
>>>
>>
>>  It's great that slimming paint can also reduce painting for caret.
>>> Previously
>>>
>> I thought of letting caret be in a composited layer to reduce painting of
>> the
>> contents under the caret.
>>
>>  I'd like to still make clear of more details about caret invalidation and
>>>
>> painting:
>>
>>  - It seems not to go through RenderObject::
>>> invalidatePaintUsingContainer(),
>>>
>> otherwise we should have invalidated the renderer in the displayItemList.
>>
>>  - When the caret is to be erased, in old painting mode we need to paint
>>> the
>>>
>> contents under the caret but don't need to paint the caret itself. How do
>> we
>> erase a caret in slimming paint mode?
>>
>>  I could look into them.
>>>
>>
>> These are good questions.
>>
>> You're correct that this does not go through
>> invalidatePaintUsingContainer. I
>> suspect it's that way because invalidatePaintUsingContainer would cause
>> the body
>> to be constantly invalidated on many pages.
>>
>> The caret display items should be removed on every update because they
>> aren't
>> cached (the clientCacheIsValid check). We might want to add a special
>> type for
>> carets to make this easier to debug, what do you think about changing the
>> display item type to be PaintPhaseForegroundCaret?
>>
>> In a slimming paint world we'll have display items for the content under
>> the
>> caret because caret drawing is "overdrawn", so I think this will just
>> work.
>>
>> https://codereview.chromium.org/800553002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Xianzhu, 2014-12-11 23:26:20]

Did more debugging, and I'm sure this is an under-invalidation issue: on
setCaretBrowsingEnabled(), the caret is missing an invalidation. As there is
also other invalidations (innerHTML += 'PASS'), we still paint HTML and the
caret in it. As HTML is not invalidated, we issue a CachedDisplayItem for the
caret, causing the assertion failure. Without the other invalidations, in normal
painting mode, we won't see the caret until the caret is scheduled to be shown
the next by the timer (when we'll get an expected invalidation).

I think instead of disabling cache for carets, we should fix the
under-invalidation issue.

Reduced paint for caret (that is, no invalidating and painting of the caret's
containing block) will be feasible after we implement per-display-item
invalidation.

https://codereview.chromium.org/800553002/diff/20001/LayoutTests/fast/text/ca...
File LayoutTests/fast/text/caret-crash.html (right):

https://codereview.chromium.org/800553002/diff/20001/LayoutTests/fast/text/ca...
LayoutTests/fast/text/caret-crash.html:5: testRunner.waitUntilDone();
if (window.testRunner)...

[Xianzhu, 2014-12-11 23:57:43]

Scratch the lgtm because we need to think more about this.

I'm tracing invalidation about caret. Based on the code, the invalidation
is through invalidatePaintUsingContainer.

Debugging why a CachedDisplayItem is created for the caret without the
original display item. It seems to me a missing invalidation issue, or the
invalidation happens through another path.

On Thu, Dec 11, 2014 at 1:48 PM, Chris Harrelson <chrishtr@chromium.org>
wrote:
>
> I also got an error. My comment:
>
> Also, how exactly do carets paint without invalidation?
>
> How much work is it for carets to start invalidating their containing
> block?
>
> On Thu, Dec 11, 2014 at 1:46 PM, Xianzhu Wang <wangxianzhu@chromium.org>
> wrote:
>
>> (Got 'Internal error' when replying in the CL, so reply the email here)
>>
>> Just noticed this and am wondering if it's a real issue: when the caret
>> is to be erased, the DrawingRecorder will generate an empty picture for the
>> caret and won't emit a new display item. It seems that we need to
>> explicitly invalidate the original display item which is for the visible
>> caret, otherwise the display item will be kept. However, for now we can
>> only invalidate by renderer. Perhaps it's time to support per-display-item
>> invalidation?
>>
>> On Thu, Dec 11, 2014 at 1:01 PM, <pdr@chromium.org> wrote:
>>>
>>> On 2014/12/11 at 20:50:04, wangxianzhu wrote:
>>>
>>>> lgtm.
>>>>
>>>
>>>  It's great that slimming paint can also reduce painting for caret.
>>>> Previously
>>>>
>>> I thought of letting caret be in a composited layer to reduce painting
>>> of the
>>> contents under the caret.
>>>
>>>  I'd like to still make clear of more details about caret invalidation
>>>> and
>>>>
>>> painting:
>>>
>>>  - It seems not to go through RenderObject::
>>>> invalidatePaintUsingContainer(),
>>>>
>>> otherwise we should have invalidated the renderer in the displayItemList.
>>>
>>>  - When the caret is to be erased, in old painting mode we need to paint
>>>> the
>>>>
>>> contents under the caret but don't need to paint the caret itself. How
>>> do we
>>> erase a caret in slimming paint mode?
>>>
>>>  I could look into them.
>>>>
>>>
>>> These are good questions.
>>>
>>> You're correct that this does not go through
>>> invalidatePaintUsingContainer. I
>>> suspect it's that way because invalidatePaintUsingContainer would cause
>>> the body
>>> to be constantly invalidated on many pages.
>>>
>>> The caret display items should be removed on every update because they
>>> aren't
>>> cached (the clientCacheIsValid check). We might want to add a special
>>> type for
>>> carets to make this easier to debug, what do you think about changing the
>>> display item type to be PaintPhaseForegroundCaret?
>>>
>>> In a slimming paint world we'll have display items for the content under
>>> the
>>> caret because caret drawing is "overdrawn", so I think this will just
>>> work.
>>>
>>> https://codereview.chromium.org/800553002/
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chrishtr, 2014-12-12 00:18:01]

https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
Source/core/paint/BlockPainter.cpp:228: RenderDrawingRecorder
recorder(paintInfo.context, &m_renderBlock, paintPhase, bounds,
DrawingRecorder::DisableCache);
Add a comment explaining the situation. Also, how exactly do carets paint
without invalidation?

How much work is it for carets to start invalidating their containing block?

[Xianzhu, 2014-12-12 00:24:16]

https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
Source/core/paint/BlockPainter.cpp:228: RenderDrawingRecorder
recorder(paintInfo.context, &m_renderBlock, paintPhase, bounds,
DrawingRecorder::DisableCache);
On 2014/12/12 00:18:01, chrishtr wrote:
> Add a comment explaining the situation. Also, how exactly do carets paint
> without invalidation?
> 
> How much work is it for carets to start invalidating their containing block?

Actually we already invalidated carets within their containing block. We just
miss the invalidation when caretBrowsing setting changes which is a common bug
of all paint modes. I'm looking into the bug.

This shows that our cached display item assertions are good to discover
under-invalidation bugs :)

[chrishtr, 2014-12-12 00:36:10]

On 2014/12/12 at 00:24:16, wangxianzhu wrote:
>
https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
> File Source/core/paint/BlockPainter.cpp (right):
> 
>
https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
> Source/core/paint/BlockPainter.cpp:228: RenderDrawingRecorder
recorder(paintInfo.context, &m_renderBlock, paintPhase, bounds,
DrawingRecorder::DisableCache);
> On 2014/12/12 00:18:01, chrishtr wrote:
> > Add a comment explaining the situation. Also, how exactly do carets paint
> > without invalidation?
> > 
> > How much work is it for carets to start invalidating their containing block?
> 
> Actually we already invalidated carets within their containing block. We just
miss the invalidation when caretBrowsing setting changes which is a common bug
of all paint modes. I'm looking into the bug.
> 
> This shows that our cached display item assertions are good to discover
under-invalidation bugs :)

Amazing! Thanks for finding the root issue.

[pdr., 2014-12-12 00:46:27]

On 2014/12/12 at 00:36:10, chrishtr wrote:
> On 2014/12/12 at 00:24:16, wangxianzhu wrote:
> >
https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
> > File Source/core/paint/BlockPainter.cpp (right):
> > 
> >
https://codereview.chromium.org/800553002/diff/20001/Source/core/paint/BlockP...
> > Source/core/paint/BlockPainter.cpp:228: RenderDrawingRecorder
recorder(paintInfo.context, &m_renderBlock, paintPhase, bounds,
DrawingRecorder::DisableCache);
> > On 2014/12/12 00:18:01, chrishtr wrote:
> > > Add a comment explaining the situation. Also, how exactly do carets paint
> > > without invalidation?
> > > 
> > > How much work is it for carets to start invalidating their containing
block?
> > 
> > Actually we already invalidated carets within their containing block. We
just miss the invalidation when caretBrowsing setting changes which is a common
bug of all paint modes. I'm looking into the bug.
> > 
> > This shows that our cached display item assertions are good to discover
under-invalidation bugs :)
> 
> Amazing! Thanks for finding the root issue.

Teamwork ftw! My analysis above was all using my reduced testcase so I didn't
see that carets actually do invalidate properly :/

One strange aspect of this is that this setting is being set asynchronously in
the first place.

wangxianzhu is going to take over landing this.