Bug 103510 - Reduce the children repaints when moved multiple times during the layout
Summary: Reduce the children repaints when moved multiple times during the layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 62048
  Show dependency treegraph
 
Reported: 2012-11-28 05:27 PST by Carlos Garcia Campos
Modified: 2012-12-05 03:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.40 KB, patch)
2012-11-28 05:33 PST, Carlos Garcia Campos
tony: review-
Details | Formatted Diff | Diff
Updated patch (10.14 KB, patch)
2012-11-30 05:55 PST, Carlos Garcia Campos
tony: review+
tony: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (9.68 KB, patch)
2012-12-03 05:27 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch for landing (10.02 KB, patch)
2012-12-03 05:39 PST, Carlos Garcia Campos
darin: review-
Details | Formatted Diff | Diff
Updated patch (10.28 KB, patch)
2012-12-04 02:12 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-11-28 05:27:39 PST
We could cache the children positions before the layout and move to the final position after the layout.
Comment 1 Carlos Garcia Campos 2012-11-28 05:33:56 PST
Created attachment 176463 [details]
Patch
Comment 2 Tony Chang 2012-11-28 10:20:02 PST
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?
Comment 3 Carlos Garcia Campos 2012-11-29 00:33:37 PST
(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.
Comment 4 Tony Chang 2012-11-29 10:04:34 PST
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.
Comment 5 Carlos Garcia Campos 2012-11-29 23:27:23 PST
(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!
Comment 6 Carlos Garcia Campos 2012-11-30 05:55:55 PST
Created attachment 176952 [details]
Updated patch

Use a Vector instead of a hash map and include a repaint test.
Comment 7 Tony Chang 2012-11-30 10:57:33 PST
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?
Comment 8 Carlos Garcia Campos 2012-12-03 03:24:37 PST
(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.
Comment 9 Carlos Garcia Campos 2012-12-03 05:27:27 PST
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.
Comment 10 Carlos Garcia Campos 2012-12-03 05:39:16 PST
Created attachment 177239 [details]
Patch for landing

Forgot to include the png in the diff, sorry.
Comment 11 Darin Adler 2012-12-03 10:34:53 PST
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.
Comment 12 Darin Adler 2012-12-03 10:35:35 PST
Syntax for inline capacity is Vector<LayoutRect, 5>, where 5 is the inline capacity.
Comment 13 Tony Chang 2012-12-03 11:38:13 PST
(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.
Comment 14 Carlos Garcia Campos 2012-12-04 01:08:41 PST
(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!
Comment 15 Carlos Garcia Campos 2012-12-04 02:12:29 PST
Created attachment 177453 [details]
Updated patch
Comment 16 Darin Adler 2012-12-04 10:15:10 PST
(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.
Comment 17 Darin Adler 2012-12-04 10:18:54 PST
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".
Comment 18 Ojan Vafai 2012-12-04 10:51:35 PST
> > 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.
Comment 19 Carlos Garcia Campos 2012-12-05 01:56:49 PST
Comment on attachment 177453 [details]
Updated patch

Committed r136656: <http://trac.webkit.org/changeset/136656>
Comment 20 Carlos Garcia Campos 2012-12-05 03:29:50 PST
(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