Bug 104366

Created attachment 178209 [details] Patch

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

> 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?

> I'm a layout expert, I'm NOT a layout expert :)

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

(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. :-)

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.

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. :(

(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.

(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.

(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 :-(

Created attachment 179926 [details] Updated patch Make the iterator mutable as suggested by Tony.

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?

Created attachment 180110 [details] Updated patch Moved the implementation of the OrderIterator class to the cpp file and made it non copyable.

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

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

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