Bug 103510 – Reduce the children repaints when moved multiple times during the layout

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We could cache the children positions before the layout and move to the final position after the layout.

[Carlos Garcia Campos]

Created attachment 176463 [details]
Patch

[Tony Chang]

Comment on attachment 176463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176463&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests. Covered by existing tests in css3/flexbox.

Can you make a repaint test that shows the improvement?

> Source/WebCore/rendering/RenderFlexibleBox.h:81
> +    typedef WTF::HashMap<const RenderBox*, LayoutRect> ChildrenFrameRect;

Can we use a Vector<LayoutRect> instead of a HashMap?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 176463 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176463&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests. Covered by existing tests in css3/flexbox.
> 
> Can you make a repaint test that shows the improvement?

Sure, I'll try.

> > Source/WebCore/rendering/RenderFlexibleBox.h:81
> > +    typedef WTF::HashMap<const RenderBox*, LayoutRect> ChildrenFrameRect;
> 
> Can we use a Vector<LayoutRect> instead of a HashMap?

I used a hash map, because I'm iterating the children using the flexbox order, I'm not sure if it's important to repaint in that order, though.

[Tony Chang]

Comment on attachment 176463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176463&action=review

> Source/WebCore/rendering/RenderFlexibleBox.cpp:385
> +        if (!selfNeedsLayout() && child->checkForRepaintDuringLayout())
> +            child->repaintDuringLayoutIfMoved(childrenOldRects.get(child));

We should also make sure that the old position is different from the current position before repainting.

>>> Source/WebCore/rendering/RenderFlexibleBox.h:81
>>> +    typedef WTF::HashMap<const RenderBox*, LayoutRect> ChildrenFrameRect;
>> 
>> Can we use a Vector<LayoutRect> instead of a HashMap?
> 
> I used a hash map, because I'm iterating the children using the flexbox order, I'm not sure if it's important to repaint in that order, though.

That's a good point, we have to iterate the children using the flexbox order.  The existing order-painting.html test might cover this?  Should be easy to test.

I guess we don't have the order iterator when you saved the child boxes. Hmm, I think we could save the child boxes right before layoutFlexItems() and I think this will still work.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 176463 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176463&action=review
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:385
> > +        if (!selfNeedsLayout() && child->checkForRepaintDuringLayout())
> > +            child->repaintDuringLayoutIfMoved(childrenOldRects.get(child));
> 
> We should also make sure that the old position is different from the current position before repainting.

That check is already done by repaintDuringLayoutIfMoved().

> >>> Source/WebCore/rendering/RenderFlexibleBox.h:81
> >>> +    typedef WTF::HashMap<const RenderBox*, LayoutRect> ChildrenFrameRect;
> >> 
> >> Can we use a Vector<LayoutRect> instead of a HashMap?
> > 
> > I used a hash map, because I'm iterating the children using the flexbox order, I'm not sure if it's important to repaint in that order, though.
> 
> That's a good point, we have to iterate the children using the flexbox order.  The existing order-painting.html test might cover this?  Should be easy to test.
> 
> I guess we don't have the order iterator when you saved the child boxes. Hmm, I think we could save the child boxes right before layoutFlexItems() and I think this will still work.

Right, I'll try that way. Thanks!

[Carlos Garcia Campos]

Created attachment 176952 [details]
Updated patch

Use a Vector instead of a hash map and include a repaint test.

[Tony Chang]

Comment on attachment 176952 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176952&action=review

> LayoutTests/css3/flexbox/repaint-column-reverse.html:23
> +    if (window.testRunner) {
> +        testRunner.waitUntilDone();
> +        testRunner.display();

Please add testRunner.dumpAsText(true) so we get the pixel result, but no render tree dump which isn't important for this test.

> LayoutTests/css3/flexbox/repaint-column-reverse.html:29
> +        document.body.appendChild(document.createTextNode(
> +            "This test checks that for flex items that are moved multiple times during the layout "
> +            + "only the initial and final positions are repainted. Only the blue flex item should be repainted "
> +            + "after changing its position. If the other flex items are repainted, this test fails."));
> +    }

I would always include this text. It makes it easier for gardeners.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:395
> +        childIndex++;

Nit: ++childIndex

> Source/WebCore/rendering/RenderFlexibleBox.cpp:735
> +    iterator.first();

Ah right, previously the constructor would do this for us. Can we also remove the call to first() in the OrderIterator constructor?

[Carlos Garcia Campos]

(In reply to comment #7)
> (From update of attachment 176952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176952&action=review

Thanks for the review.

> > LayoutTests/css3/flexbox/repaint-column-reverse.html:23
> > +    if (window.testRunner) {
> > +        testRunner.waitUntilDone();
> > +        testRunner.display();
> 
> Please add testRunner.dumpAsText(true) so we get the pixel result, but no render tree dump which isn't important for this test.

Sure.

> > LayoutTests/css3/flexbox/repaint-column-reverse.html:29
> > +        document.body.appendChild(document.createTextNode(
> > +            "This test checks that for flex items that are moved multiple times during the layout "
> > +            + "only the initial and final positions are repainted. Only the blue flex item should be repainted "
> > +            + "after changing its position. If the other flex items are repainted, this test fails."));
> > +    }
> 
> I would always include this text. It makes it easier for gardeners.

Ok, I was trying to avoid adding text to a pixel test, I won't be able to add the ref image indeed, because my system renders font slightly different.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:395
> > +        childIndex++;
> 
> Nit: ++childIndex

Ok.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:735
> > +    iterator.first();
> 
> Ah right, previously the constructor would do this for us. Can we also remove the call to first() in the OrderIterator constructor?

I'll check.

[Carlos Garcia Campos]

Created attachment 177236 [details]
Patch for landing

Addressed review comments. I haven't included the text in the test in the end to avoid problems with text rendering.

[Carlos Garcia Campos]

Created attachment 177239 [details]
Patch for landing

Forgot to include the png in the diff, sorry.

[Darin Adler]

Comment on attachment 177239 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=177239&action=review

Looks pretty good. Needs a couple small improvements.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:336
> +    WTF::Vector<LayoutRect> childrenOldRects;

Should be just Vector. Not WTF::Vector. Also, we should choose some amount of inline capacity to cut down on memory allocation and make performance better. A number large enough that it’s often larger than the number of children, but not so large that we use a ton of stack space.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:370
> +void RenderFlexibleBox::childrenFrameRects(WTF::Vector<LayoutRect>& childrenFrameRects)

No WTF prefix needed here.

A function like this that uses an out parameter should have “get” in its name. Or “append” to make clear its contract is that it appends to an existing vector.

[Darin Adler]

Syntax for inline capacity is Vector<LayoutRect, 5>, where 5 is the inline capacity.

[Tony Chang]

(In reply to comment #8)
> Ok, I was trying to avoid adding text to a pixel test, I won't be able to add the ref image indeed, because my system renders font slightly different.

Oh, I see.  Ok, keeping the text out of the test is fine.  It may also avoid having to generate separate results for different versions of OS X as anti-aliasing settings change.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 177239 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177239&action=review
> 
> Looks pretty good. Needs a couple small improvements.
> 
> > Source/WebCore/rendering/RenderFlexibleBox.cpp:336
> > +    WTF::Vector<LayoutRect> childrenOldRects;
> 
> Should be just Vector. Not WTF::Vector. Also, we should choose some amount of inline capacity to cut down on memory allocation and make performance better. A number large enough that it’s often larger than the number of children, but not so large that we use a ton of stack space.

This is a good point, we should probably use the same number for the childSizes vector. I think we could use 8. 

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:370
> > +void RenderFlexibleBox::childrenFrameRects(WTF::Vector<LayoutRect>& childrenFrameRects)
> 
> No WTF prefix needed here.

Yes, I thought the same, but I saw the WTF prefix was used everywhere in RenderFlexibleBox, so I used it for consistency. I'll remove it.

> A function like this that uses an out parameter should have “get” in its name. Or “append” to make clear its contract is that it appends to an existing vector.

Ok, I'll use add or append then, thanks!

[Carlos Garcia Campos]

Created attachment 177453 [details]
Updated patch

[Darin Adler]

(In reply to comment #14)
> I saw the WTF prefix was used everywhere in RenderFlexibleBox, so I used it for consistency.

Someone should fix that mistake in RenderFlexibleBox.

[Darin Adler]

Comment on attachment 177453 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177453&action=review

> Source/WebCore/rendering/RenderFlexibleBox.cpp:338
> +    ChildrenFrameRects childrenOldRects;
> +    appendChildrenFrameRects(childrenOldRects);
>      layoutFlexItems(*m_orderIterator, lineContexts);

It’s strange that layoutFlexItems receives the iterator as an argument, but appendChildrenFrameRects and repaintChildrenDuringLayoutIfMoved both get the iterator directly from the data member. The three functions should do this consistently.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:396
> +}

I think we should add this here:

    ASSERT(childIndex == childrenOldRects.size());

> Source/WebCore/rendering/RenderFlexibleBox.h:81
> +    typedef Vector<LayoutRect, 8> ChildrenFrameRects;

Needs a comment explaining where the number 8 came from.

I think the name ChildFrameRects would be better. Since rects is plural, children doesn’t also need to be plural. In English it would be "child frame rectangles" not "children frame rectangles".

[Ojan Vafai]

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:338
> > +    ChildrenFrameRects childrenOldRects;
> > +    appendChildrenFrameRects(childrenOldRects);
> >      layoutFlexItems(*m_orderIterator, lineContexts);
> 
> It’s strange that layoutFlexItems receives the iterator as an argument, but appendChildrenFrameRects and repaintChildrenDuringLayoutIfMoved both get the iterator directly from the data member. The three functions should do this consistently.

We should change layoutFlexItems, et al, to always use the member. It didn't used to be a member and we just never did the cleanup to stop passing it as an argument.

[Carlos Garcia Campos]

Comment on attachment 177453 [details]
Updated patch

Committed r136656: <http://trac.webkit.org/changeset/136656>

[Carlos Garcia Campos]

(In reply to comment #16)
> (In reply to comment #14)
> > I saw the WTF prefix was used everywhere in RenderFlexibleBox, so I used it for consistency.
> 
> Someone should fix that mistake in RenderFlexibleBox.

https://bugs.webkit.org/show_bug.cgi?id=104109