Bug 196841 - Regression(r237903) Speedometer 2 is 1-2% regressed on iOS
Summary: Regression(r237903) Speedometer 2 is 1-2% regressed on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 190774
  Show dependency treegraph
 
Reported: 2019-04-11 17:44 PDT by Chris Dumez
Modified: 2019-04-15 15:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.78 KB, patch)
2019-04-11 17:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.48 KB, patch)
2019-04-15 08:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.65 MB, application/zip)
2019-04-15 09:33 PDT, Build Bot
no flags Details
Patch (2.80 KB, patch)
2019-04-15 09:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-04-11 17:44:15 PDT
PLT is 1-2% regressed on iOS after r237903.
Comment 1 Chris Dumez 2019-04-11 17:44:30 PDT
<rdar://problem/45957016>
Comment 2 Chris Dumez 2019-04-11 17:51:43 PDT
Created attachment 367270 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-04-11 17:54:52 PDT
Comment on attachment 367270 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.cpp:548
> +        if (textUnderlinePosition() == TextUnderlinePosition::Under || other.textUnderlinePosition() == TextUnderlinePosition::Under)
> +            return true;

Can FromFont values also trigger visual overflow?
Comment 4 Myles C. Maxfield 2019-04-11 19:10:32 PDT
Comment on attachment 367270 [details]
Patch

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

>> Source/WebCore/rendering/style/RenderStyle.cpp:548
>> +            return true;
> 
> Can FromFont values also trigger visual overflow?

FromFont can trigger visual overflow, but any Length value can also trigger visual overflow. If the tests don't catch this, then I need to write a new test that this patch would break.

If this patch fixes the regression, it means that underline rects occur outside of the text bounds, which shouldn't be true for regularly-sized text with regular outlines. I don't think our performance tests try to change the type of underlines, so this probably isn't the right fix.
Comment 5 Myles C. Maxfield 2019-04-11 19:12:35 PDT
See also: https://bugs.webkit.org/show_bug.cgi?id=196158 We have at least one other bug about the repaint rects being wrong for outlines. Fixing the repaint rects so they are properly placed/sized would likely make them entirely contained within their text, which would mean the repaint rects wouldn't be affected by the underlines, which means it (hopefully) would fix this performance regression.
Comment 6 Chris Dumez 2019-04-12 09:13:50 PDT
Comment on attachment 367270 [details]
Patch

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

>>> Source/WebCore/rendering/style/RenderStyle.cpp:548
>>> +            return true;
>> 
>> Can FromFont values also trigger visual overflow?
> 
> FromFont can trigger visual overflow, but any Length value can also trigger visual overflow. If the tests don't catch this, then I need to write a new test that this patch would break.
> 
> If this patch fixes the regression, it means that underline rects occur outside of the text bounds, which shouldn't be true for regularly-sized text with regular outlines. I don't think our performance tests try to change the type of underlines, so this probably isn't the right fix.

"I don't think our performance tests try to change the type of underlines"

What do you mean by type of underline? What would it look like from the web side? (e.g. css) I could check Speedometer. I do know that Speedometer underlines text although this may not be what you meant.
Comment 7 Myles C. Maxfield 2019-04-12 12:52:50 PDT
(In reply to Chris Dumez from comment #6)
> Comment on attachment 367270 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367270&action=review
> 
> >>> Source/WebCore/rendering/style/RenderStyle.cpp:548
> >>> +            return true;
> >> 
> >> Can FromFont values also trigger visual overflow?
> > 
> > FromFont can trigger visual overflow, but any Length value can also trigger visual overflow. If the tests don't catch this, then I need to write a new test that this patch would break.
> > 
> > If this patch fixes the regression, it means that underline rects occur outside of the text bounds, which shouldn't be true for regularly-sized text with regular outlines. I don't think our performance tests try to change the type of underlines, so this probably isn't the right fix.
> 
> "I don't think our performance tests try to change the type of underlines"
> 
> What do you mean by type of underline? What would it look like from the web
> side? (e.g. css) I could check Speedometer. I do know that Speedometer
> underlines text although this may not be what you meant.

Using text-decoration-thickness, text-decoration-width, text-underline-offset, or text-underline-position.
Comment 8 Myles C. Maxfield 2019-04-14 22:10:05 PDT
Comment on attachment 367270 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyle.cpp:549
> +        return visualOverflowForDecorations(*this, nullptr) != visualOverflowForDecorations(other, nullptr);

I see what was happening. Before r237903, we were running this "visualOverflowForDecorations() != visualOverflowForDecorations()" code. visualOverflowForDecorations() returns a GlyphOverflow, which contains 4 values, each of which represents how far to explode the overflow rect from the layout overflow rect. When visualOverflowForDecorations() builds up its return value, the GlyphOverflow values start at 0, and extendIntToFloat() pipes everything through max(), so that means an underline that is entirely contained within the layout overflow rect would return {0, 0, 0, 0}, which is the same as what would be returned if there was no decoration at all. Therefore, when the style change is "add an underline that exists entirely within the layout rect" or "remove such an underline", visualOverflowForDecoration() returns the same value, and RenderStyle::changeAffectsVisualOverflow() says that this change doesn't affect visual overflow. This is correct and desirable.

We should preserve this "only calculate rect diffs if they occur outside of the layout rect" behavior. I think the best patch would remove the check on lines 547-548 (because visualOverflowForDecorations() will handle that) and, in the return statement on line 549, each call to visualOverflowForDecoration() would be unioned with the layout rect before doing the inequality comparison.
Comment 9 Myles C. Maxfield 2019-04-14 22:15:08 PDT
Comment on attachment 367270 [details]
Patch

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

>> Source/WebCore/rendering/style/RenderStyle.cpp:549
>> +        return visualOverflowForDecorations(*this, nullptr) != visualOverflowForDecorations(other, nullptr);
> 
> I see what was happening. Before r237903, we were running this "visualOverflowForDecorations() != visualOverflowForDecorations()" code. visualOverflowForDecorations() returns a GlyphOverflow, which contains 4 values, each of which represents how far to explode the overflow rect from the layout overflow rect. When visualOverflowForDecorations() builds up its return value, the GlyphOverflow values start at 0, and extendIntToFloat() pipes everything through max(), so that means an underline that is entirely contained within the layout overflow rect would return {0, 0, 0, 0}, which is the same as what would be returned if there was no decoration at all. Therefore, when the style change is "add an underline that exists entirely within the layout rect" or "remove such an underline", visualOverflowForDecoration() returns the same value, and RenderStyle::changeAffectsVisualOverflow() says that this change doesn't affect visual overflow. This is correct and desirable.
> 
> We should preserve this "only calculate rect diffs if they occur outside of the layout rect" behavior. I think the best patch would remove the check on lines 547-548 (because visualOverflowForDecorations() will handle that) and, in the return statement on line 549, each call to visualOverflowForDecoration() would be unioned with the layout rect before doing the inequality comparison.

Oh, we don't need to do this unioning if we keep calling visualOverflowForDecorations, which does the max() behavior described above. So, yeah, I think the best patch is the same as this but without lines 545-548.
Comment 10 Chris Dumez 2019-04-15 08:28:18 PDT
Created attachment 367415 [details]
Patch
Comment 11 Chris Dumez 2019-04-15 09:27:40 PDT
(In reply to Myles C. Maxfield from comment #9)
> Comment on attachment 367270 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367270&action=review
> 
> >> Source/WebCore/rendering/style/RenderStyle.cpp:549
> >> +        return visualOverflowForDecorations(*this, nullptr) != visualOverflowForDecorations(other, nullptr);
> > 
> > I see what was happening. Before r237903, we were running this "visualOverflowForDecorations() != visualOverflowForDecorations()" code. visualOverflowForDecorations() returns a GlyphOverflow, which contains 4 values, each of which represents how far to explode the overflow rect from the layout overflow rect. When visualOverflowForDecorations() builds up its return value, the GlyphOverflow values start at 0, and extendIntToFloat() pipes everything through max(), so that means an underline that is entirely contained within the layout overflow rect would return {0, 0, 0, 0}, which is the same as what would be returned if there was no decoration at all. Therefore, when the style change is "add an underline that exists entirely within the layout rect" or "remove such an underline", visualOverflowForDecoration() returns the same value, and RenderStyle::changeAffectsVisualOverflow() says that this change doesn't affect visual overflow. This is correct and desirable.
> > 
> > We should preserve this "only calculate rect diffs if they occur outside of the layout rect" behavior. I think the best patch would remove the check on lines 547-548 (because visualOverflowForDecorations() will handle that) and, in the return statement on line 549, each call to visualOverflowForDecoration() would be unioned with the layout rect before doing the inequality comparison.
> 
> Oh, we don't need to do this unioning if we keep calling
> visualOverflowForDecorations, which does the max() behavior described above.
> So, yeah, I think the best patch is the same as this but without lines
> 545-548.

Looks like the suggested change may be causing test failures:
Regressions: Unexpected crashes (3)
  fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-altered.html [ Crash ]
  fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-removed.html [ Crash ]
  fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect.html [ Crash ]
Comment 12 Build Bot 2019-04-15 09:33:38 PDT
Comment on attachment 367415 [details]
Patch

Attachment 367415 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11871465

New failing tests:
fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect.html
fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-altered.html
fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-removed.html
Comment 13 Build Bot 2019-04-15 09:33:40 PDT
Created attachment 367417 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 14 Chris Dumez 2019-04-15 09:39:06 PDT
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000018
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

VM Regions Near 0x18:
--> 
    __TEXT                 0000000100f96000-0000000100ff3000 [  372K] r-x/rwx SM=COW  6 [/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DumpRenderTree]

Application Specific Information:
CRASHING TEST: fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-altered.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000105bfcbe4 WebCore::InlineBox::root() const + 4 (InlineBox.h:156)
1   com.apple.WebCore             	0x0000000105e27f92 WebCore::computeUnderlineOffset(WebCore::TextUnderlinePosition, WebCore::TextUnderlineOffset, WebCore::FontMetrics const&, WebCore::InlineTextBox const*, float) + 242 (InlineTextBoxStyle.cpp:69)
2   com.apple.WebCore             	0x0000000105e2831f WebCore::visualOverflowForDecorations(WebCore::RenderStyle const&, WebCore::InlineTextBox const*) + 463 (InlineTextBoxStyle.cpp:135)
3   com.apple.WebCore             	0x0000000105dc677b WebCore::RenderStyle::changeAffectsVisualOverflow(WebCore::RenderStyle const&) const + 203 (FontCascade.h:74)
4   com.apple.WebCore             	0x0000000105dc5844 WebCore::RenderStyle::changeRequiresLayout(WebCore::RenderStyle const&, WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const + 1076 (RenderStyle.cpp:738)
5   com.apple.WebCore             	0x0000000105dc79e0 WebCore::RenderStyle::diff(WebCore::RenderStyle const&, WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const + 96 (RenderStyle.cpp:1094)
6   com.apple.WebCore             	0x0000000105c7f58e WebCore::RenderElement::setStyle(WebCore::RenderStyle&&, WebCore::StyleDifference) + 62 (RenderElement.cpp:402)
7   com.apple.WebCore             	0x0000000105e1e371 WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&) + 689 (RenderTreeUpdater.cpp:294)
8   com.apple.WebCore             	0x0000000105e1d6bd WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 1037 (RenderTreeUpdater.cpp:191)
9   com.apple.WebCore             	0x0000000105e1d21b WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 587
10  com.apple.WebCore             	0x00000001054d1c0d WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 989 (memory:2595)
11  com.apple.WebCore             	0x00000001054d2619 WebCore::Document::updateStyleIfNeeded() + 329 (Document.cpp:2037)
12  com.apple.WebCore             	0x000000010595ff01 WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() + 1233
13  com.apple.WebKitLegacy        	0x00000001085fd5af -[WebHTMLView(WebPrivate) viewWillDraw] + 79 (WebHTMLView.mm:1595)
Comment 15 Chris Dumez 2019-04-15 09:40:34 PDT
(In reply to Chris Dumez from comment #14)
> Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
> Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000018
> Exception Note:        EXC_CORPSE_NOTIFY
> 
> Termination Signal:    Segmentation fault: 11
> Termination Reason:    Namespace SIGNAL, Code 0xb
> Terminating Process:   exc handler [0]
> 
> VM Regions Near 0x18:
> --> 
>     __TEXT                 0000000100f96000-0000000100ff3000 [  372K]
> r-x/rwx SM=COW  6
> [/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DumpRenderTree]
> 
> Application Specific Information:
> CRASHING TEST:
> fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-
> altered.html
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebCore             	0x0000000105bfcbe4
> WebCore::InlineBox::root() const + 4 (InlineBox.h:156)
> 1   com.apple.WebCore             	0x0000000105e27f92
> WebCore::computeUnderlineOffset(WebCore::TextUnderlinePosition,
> WebCore::TextUnderlineOffset, WebCore::FontMetrics const&,
> WebCore::InlineTextBox const*, float) + 242 (InlineTextBoxStyle.cpp:69)
> 2   com.apple.WebCore             	0x0000000105e2831f
> WebCore::visualOverflowForDecorations(WebCore::RenderStyle const&,
> WebCore::InlineTextBox const*) + 463 (InlineTextBoxStyle.cpp:135)
> 3   com.apple.WebCore             	0x0000000105dc677b
> WebCore::RenderStyle::changeAffectsVisualOverflow(WebCore::RenderStyle
> const&) const + 203 (FontCascade.h:74)
> 4   com.apple.WebCore             	0x0000000105dc5844
> WebCore::RenderStyle::changeRequiresLayout(WebCore::RenderStyle const&,
> WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const +
> 1076 (RenderStyle.cpp:738)
> 5   com.apple.WebCore             	0x0000000105dc79e0
> WebCore::RenderStyle::diff(WebCore::RenderStyle const&,
> WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const +
> 96 (RenderStyle.cpp:1094)
> 6   com.apple.WebCore             	0x0000000105c7f58e
> WebCore::RenderElement::setStyle(WebCore::RenderStyle&&,
> WebCore::StyleDifference) + 62 (RenderElement.cpp:402)
> 7   com.apple.WebCore             	0x0000000105e1e371
> WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&,
> WebCore::Style::ElementUpdate const&) + 689 (RenderTreeUpdater.cpp:294)
> 8   com.apple.WebCore             	0x0000000105e1d6bd
> WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 1037
> (RenderTreeUpdater.cpp:191)
> 9   com.apple.WebCore             	0x0000000105e1d21b
> WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::
> Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 587
> 10  com.apple.WebCore             	0x00000001054d1c0d
> WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 989
> (memory:2595)
> 11  com.apple.WebCore             	0x00000001054d2619
> WebCore::Document::updateStyleIfNeeded() + 329 (Document.cpp:2037)
> 12  com.apple.WebCore             	0x000000010595ff01
> WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() + 1233
> 13  com.apple.WebKitLegacy        	0x00000001085fd5af
> -[WebHTMLView(WebPrivate) viewWillDraw] + 79 (WebHTMLView.mm:1595)

It is not OK to call visualOverflowForDecorations() with TextUnderlinePosition::Under and a null inlineTextBox. I'll go back to my original code.
Comment 16 Chris Dumez 2019-04-15 09:43:26 PDT
Created attachment 367418 [details]
Patch
Comment 17 WebKit Commit Bot 2019-04-15 11:56:57 PDT
Comment on attachment 367418 [details]
Patch

Clearing flags on attachment: 367418

Committed r244277: <https://trac.webkit.org/changeset/244277>
Comment 18 WebKit Commit Bot 2019-04-15 11:56:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Myles C. Maxfield 2019-04-15 15:22:08 PDT
Thanks for fixing this, Chris!