Bug 103665 – [mathml] Improve performance of nested sup or sub elements

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

[mathml] Improve preformance of nested sup or sub elements

[Tony Chang]

Created attachment 176806 [details]
Patch

[Tony Chang]

I found this while investigating bug 102585.

[Darin Adler]

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?

[Tony Chang]

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.

[Darin Adler]

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?

[Darin Adler]

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?

[Tony Chang]

Created attachment 176876 [details]
Patch

[Tony Chang]

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

[Eric Seidel (no email)]

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?

[Tony Chang]

Created attachment 177075 [details]
Patch

[Tony Chang]

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

[Darin Adler]

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

[Darin Adler]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Tony Chang]

Created attachment 177278 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 177278 [details]
Patch for landing

Clearing flags on attachment: 177278

Committed r136409: <http://trac.webkit.org/changeset/136409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.