Bug 104366 – Make order iterator member stack allocated in RenderFlexibleBox

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Carlos Garcia Campos]

We avoid heap allocations during layout.

[Carlos Garcia Campos]

Created attachment 178209 [details]
Patch

[Build Bot]

Comment on attachment 178209 [details]
Patch

Attachment 178209 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15187298

[Alexey Proskuryakov]

> We avoid heap allocations during layout.

I'm a layout expert, but this is the first time I hear about this rule. Can you please elaborate?

Will this make RenderFlexibleBox much larger in some cases, or does m_orderIterator always get created eventually anyway?

[Alexey Proskuryakov]

> I'm a layout expert,

I'm NOT a layout expert :)

[Andreas Kling]

What benchmark (or web page) are you targeting? How big of an improvement is this?

[Eric Seidel (no email)]

(In reply to comment #3)
> > We avoid heap allocations during layout.
> 
> I'm a layout expert, but this is the first time I hear about this rule. Can you please elaborate?

We do lots of heap allocations during layout :-)  I haven't read the patch yet. But that rule as stated is overbroad. :-)

[Tony Chang]

I don't think Carlos is saying that avoiding heap allocations during layout is a rule.  He's just saying for this code, we'll avoid a heap allocation since we always create this object.  He is simply describing the change.

[Ojan Vafai]

Comment on attachment 178209 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        (WebCore::RenderFlexibleBox::baselinePosition): Make it non const
> +        because the order iterator is modified.

This doesn't work because now it's not actually overriding the parent class's virtual baselinePosition method. constness has to match. I'm not what you should do instead. :(

[Tony Chang]

(In reply to comment #8)
> (From update of attachment 178209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178209&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        (WebCore::RenderFlexibleBox::baselinePosition): Make it non const
> > +        because the order iterator is modified.
> 
> This doesn't work because now it's not actually overriding the parent class's virtual baselinePosition method. constness has to match. I'm not what you should do instead. :(

I would probably make m_orderIterator mutable.

[Carlos Garcia Campos]

(In reply to comment #7)
> I don't think Carlos is saying that avoiding heap allocations during layout is a rule.  He's just saying for this code, we'll avoid a heap allocation since we always create this object.  He is simply describing the change.

Yes, exactly, the iterator was not a member at first, then it became a member and Tony suggested that maybe we could make it stack allocated, which I think makes sense, so I wrote the patch. This doesn't try to fix a performance issue in the layout algorithm.

[Carlos Garcia Campos]

(In reply to comment #8)
> (From update of attachment 178209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178209&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        (WebCore::RenderFlexibleBox::baselinePosition): Make it non const
> > +        because the order iterator is modified.
> 
> This doesn't work because now it's not actually overriding the parent class's virtual baselinePosition method. constness has to match. I'm not what you should do instead. :(

Yes, I noticed it when I saw the windows build failed :-(

[Carlos Garcia Campos]

Created attachment 179926 [details]
Updated patch

Make the iterator mutable as suggested by Tony.

[Tony Chang]

Comment on attachment 179926 [details]
Updated patch

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

> Source/WebCore/ChangeLog:8
> +        It avoids heap allocations during the layout.

Nit: I would say, "It avoids a heap allocation during layout."

> Source/WebCore/rendering/RenderFlexibleBox.h:92
> +        void setOrderValues(const OrderHashSet& orderValues)
> +        {
> +            reset();

Can we move this method definition into the .cpp file? That would also let you keep OrderHashTraits in the .cpp file.

> Source/WebCore/rendering/RenderFlexibleBox.h:107
> +        RenderBox* next()
> +        {
> +            do {

Can we move this method definition into the .cpp file?

> Source/WebCore/rendering/RenderFlexibleBox.h:133
> +    private:
> +        RenderFlexibleBox* m_flexibleBox;

Maybe add WTF_MAKE_NONCOPYABLE(OrderIterator) to avoid accidental copies?

[Carlos Garcia Campos]

Created attachment 180110 [details]
Updated patch

Moved the implementation of the OrderIterator class to the cpp file and made it non copyable.

[Carlos Garcia Campos]

Created attachment 180121 [details]
Updated patch to apply on current git master

[Tony Chang]

Comment on attachment 180121 [details]
Updated patch to apply on current git master

Thanks!

[Carlos Garcia Campos]

Committed r138235: <http://trac.webkit.org/changeset/138235>