REGRESSION(r162837): 5% regression on html5-full-render and 3% regression in DoYouEvenBench

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

See https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D

We have 3% regression there.  The regression range is http://trac.webkit.org/log/?rev=162838&stop_rev=162835&verbose=on
and
r162835 is specific to GTK+
r162836 is TextTrack cleanup but DoYouEvenBench doesn't use media elements as far as I know
r162838 is just a TestExpectations update.

[Radar WebKit Bug Importer]

<rdar://problem/15920243>

[Ryosuke Niwa]

We also seem to have 5-6% regression on html5-full-render as well:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mountainlion%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%5D

[Andreas Kling]

We may need to keep some of the paint throttling logic for WK1.

[Antti Koivisto]

This is likely just WK1. We don't need to keep throttling logic (it was never enabled in Mac anyway), but we may need to do some repaint rect unioning so we don't call to NSView land that much.

[Antti Koivisto]

Created attachment 222466 [details]
patch

[WebKit Commit Bot]

Attachment 222466 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.cpp:1269:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 222466 [details]
patch

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

> Source/WebCore/dom/Document.cpp:4317
>  Document* Document::topDocument() const
>  {
> -    // FIXME: Why does this walk up owner elements instead using the frame tree as parentDocument does?
> -    // The frame tree even has a top() function.
> -    Document* document = const_cast<Document*>(this);
> -    while (Element* element = document->ownerElement())
> -        document = &element->document();
> -    return document;
> +    return m_frame ? m_frame->mainFrame().document() : const_cast<Document*>(this);
>  }

How is this change related to the rest of the patch? Does something depend on it?

Looks like this never returns null, so it should be changed to return a reference.

> Source/WebCore/rendering/RenderView.h:238
> +    class RepaintRegionAccumulator {

This has no members that are not copyable; we should probably mark it non-copyable explicitly.

[Antti Koivisto]

> How is this change related to the rest of the patch? Does something depend on it?

Not really except that I call it and noticed it was silly.

> Looks like this never returns null, so it should be changed to return a reference.

Yeah, later.

[Andreas Kling]

Comment on attachment 222466 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        Accumulate repaint region in RendeView instead of flushing rects directly to the system.

RenderView

> Source/WebCore/rendering/RenderView.h:240
> +        RepaintRegionAccumulator(RenderView*);

explicit

[Antti Koivisto]

https://trac.webkit.org/r162947

[Alexey Proskuryakov]

This change broke multiple repaint tests: <http://trac.webkit.org/changeset/162947>.

For a patch like this, waiting for EWS would have been very useful.

[Alexey Proskuryakov]

This caused timeouts on two flexbox tests, filed bug 127809.

[David Kilzer (:ddkilzer)]

(In reply to comment #12)
> This caused timeouts on two flexbox tests, filed bug 127809.

This also caused Bug 129104.