[mathml] Improve preformance of nested sup or sub elements
Created attachment 176806 [details] Patch
I found this while investigating bug 102585.
Comment on attachment 176806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176806&action=review > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:163 > + if (oldPaddingTop == baseWrapper->style()->paddingTop() && oldPaddingBottom == baseWrapper->style()->paddingBottom()) > + return; Is it OK to skip the call to RenderMathMLBlock::layout() in this case?
Comment on attachment 176806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176806&action=review >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:163 >> + return; > > Is it OK to skip the call to RenderMathMLBlock::layout() in this case? Yes, see the top of this function where we called RenderMathMLBlock::layout the first time. This is avoiding the second call to layout since nothing has changed.
Comment on attachment 176806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176806&action=review > Source/WebCore/ChangeLog:3 > + [mathml] Improve preformance of nested sup or sub elements Typo: preformance > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:139 > + Length oldPaddingTop = baseWrapper->style()->paddingTop(); > + Length oldPaddingBottom = baseWrapper->style()->paddingBottom(); I’d probably use a “needsLayout” boolean and check that the old value matches the new value right at the point where the set call is, rather than caching the old values, but this way is OK too. > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:140 > + ASSERT(baseWrapper->style()->refCount() == 1); There’s a function for this kind of check called isOnlyRef() that’s better than an explicit check for 1. >>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:163 >>> + return; >> >> Is it OK to skip the call to RenderMathMLBlock::layout() in this case? > > Yes, see the top of this function where we called RenderMathMLBlock::layout the first time. This is avoiding the second call to layout since nothing has changed. Wow, that’s a really crazy pattern, calling the base class layout function twice! > LayoutTests/ChangeLog:3 > + [mathml] Improve preformance of nested sup or sub elements Typo: preformance > LayoutTests/ChangeLog:22 > + * platform/chromium-linux/mathml/presentation/roots-expected.txt: > + * platform/chromium-linux/mathml/presentation/row-alignment-expected.txt: > + * platform/chromium-linux/mathml/presentation/sub-expected.txt: > + * platform/chromium-linux/mathml/presentation/subsup-expected.txt: > + * platform/chromium-linux/mathml/presentation/sup-expected.txt: > + * platform/chromium-linux/mathml/presentation/tables-expected.txt: What is going on here? If this patch is simply a performance optimization, why is it changing test results?
Created attachment 176876 [details] Patch
(In reply to comment #6) > (From update of attachment 176806 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176806&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:139 > > + Length oldPaddingTop = baseWrapper->style()->paddingTop(); > > + Length oldPaddingBottom = baseWrapper->style()->paddingBottom(); > > I’d probably use a “needsLayout” boolean and check that the old value matches the new value right at the point where the set call is, rather than caching the old values, but this way is OK too. Switched to use a needsLayout bool. > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:140 > > + ASSERT(baseWrapper->style()->refCount() == 1); > > There’s a function for this kind of check called isOnlyRef() that’s better than an explicit check for 1. Switched to hasOneRef(). > > LayoutTests/ChangeLog:22 > > + * platform/chromium-linux/mathml/presentation/roots-expected.txt: > > + * platform/chromium-linux/mathml/presentation/row-alignment-expected.txt: > > + * platform/chromium-linux/mathml/presentation/sub-expected.txt: > > + * platform/chromium-linux/mathml/presentation/subsup-expected.txt: > > + * platform/chromium-linux/mathml/presentation/sup-expected.txt: > > + * platform/chromium-linux/mathml/presentation/tables-expected.txt: > > What is going on here? If this patch is simply a performance optimization, why is it changing test results? See the comment in the ChangeLog about the mathml.css change. By default, flex items stretch, which requires an extra layout. By switching to align at the top or bottom, we avoid this extra layout. Visually, the pixel results are the same, just the size of some RenderBoxes that contain the superscripts and subscripts are smaller. Let me know how to better explain this in the ChangeLog.
Comment on attachment 176876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176876&action=review > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:138 > + ASSERT(baseWrapper->style()->hasOneRef()); I think you mean ->unique(), not hasOneRef(). > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:139 > + bool needsLayout = false; Should this be needsSecondLayout? > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:175 > + baseWrapper->setChildNeedsLayout(true, MarkOnlyThis); Can you explain the change from needsLayotu to needsChildLayout ehre?
Created attachment 177075 [details] Patch
(In reply to comment #9) > (From update of attachment 176876 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176876&action=review > > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:138 > > + ASSERT(baseWrapper->style()->hasOneRef()); > > I think you mean ->unique(), not hasOneRef(). I tried that and it crashes. It looks like unique() is a special flag that is only set by the StyleResolver. In this case, since it's the style of an anonymous mathml block, I think it will always be unique. > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:139 > > + bool needsLayout = false; > > Should this be needsSecondLayout? Sure, renamed. > > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:175 > > + baseWrapper->setChildNeedsLayout(true, MarkOnlyThis); > > Can you explain the change from needsLayotu to needsChildLayout ehre? My understanding is that you call setChildNeedsLayout on children and in this case, baseWrapper is a child of |this|. I also changed the setChildNeedsLayout call to setNeedsLayout since it was being called on |this|.
Comment on attachment 176876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176876&action=review >>> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:138 >>> + ASSERT(baseWrapper->style()->hasOneRef()); >> >> I think you mean ->unique(), not hasOneRef(). > > I tried that and it crashes. It looks like unique() is a special flag that is only set by the StyleResolver. In this case, since it's the style of an anonymous mathml block, I think it will always be unique. Why did you think I meant unique()? I meant hasOneRef(), specifically RefCounted::hasOneRef().
Comment on attachment 177075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177075&action=review > Source/WebCore/ChangeLog:15 > + (msup): Stretching children also cause extra layouts. Avoid this by aligning to the top. Should be "causes", not "cause". > LayoutTests/ChangeLog:9 > + The pixel results should be the same, this is only a change to the render tree. This is the perfect place to explain why the render tree changed; saying the changes in MathML’s CSS file affects the position of the boxes, but not painting or hit testing. If that’s correct.
(In reply to comment #12) > (From update of attachment 176876 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=176876&action=review > > Why did you think I meant unique()? I meant hasOneRef(), specifically RefCounted::hasOneRef(). I was referencing an earlier (offline) conversation that Tony and I had (before this bug even was opened). I didn't see your comment. :)
I had mentioned to him during that conversation that modifying RenderStyle as we were doing here is unsafe in the general case, and so we should ASSERT that the style was unique. I was not aware that isUnique() was not the way to do that. Obviously hasOnlyOneRef() works too, and it appears is to be the only way to do what I had meant in that earlier conversation.
Created attachment 177278 [details] Patch for landing
Comment on attachment 177278 [details] Patch for landing Clearing flags on attachment: 177278 Committed r136409: <http://trac.webkit.org/changeset/136409>
All reviewed patches have been landed. Closing bug.