Closed Bug 1521931 Opened 3 years ago Closed 3 years ago

Parser should yield if there is a pending high priority event before first contentful paint

Categories

(Core :: DOM: HTML Parser, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

In order get first contentful paint sooner, we shouldn't try to continue synchronously create DOM if there is a pending vsync.

Summary: Parser should yield if there is a pending high priority event before first contentful patin → Parser should yield if there is a pending high priority event before first contentful paint
Priority: -- → P2
Comment on attachment 9038362 [details] [diff] [review]
parser_blocking_child_only.diff

Let's see what Henri thinks about this. The idea is that we really must paint asap if we haven't got first contentful paint yet.
Especially we need to try to paint _before_ some slow scripts.

(bug 1521955 is about after scripts basically, but that seems to cause still more test failures to look at)
Attachment #9038362 - Flags: review?(hsivonen)
Comment on attachment 9038362 [details] [diff] [review]
parser_blocking_child_only.diff

Review of attachment 9038362 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if the requested comments are added to the code

::: parser/html/nsHtml5StreamParser.cpp
@@ +127,5 @@
> +                doc->Dispatch(TaskCategory::Network, flusher.forget()))) {
> +          PROFILER_ADD_MARKER("HighPrio blocking parser flushing(1)", DOM);
> +          return NS_OK;
> +        }
> +      }

Please add a comment that the code is here and not at the top of RunFlushLoop() in order to reuse the runnable.

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +61,5 @@
> +              doc->Dispatch(TaskCategory::Network, flusher.forget()))) {
> +        PROFILER_ADD_MARKER("HighPrio blocking parser flushing(2)", DOM);
> +        return NS_OK;
> +      }
> +    }

Please add a comment that the code is here and not at the top of RunFlushLoop() in order to reuse the runnable.
Attachment #9038362 - Flags: review?(hsivonen) → review+

ok, will do, thanks.

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/869a4e8b520b
Parser should yield if there is a pending high priority event before first contentful paint, r=hsivonen
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

We won some very big performance improvements thanks to this bug! \0/

== Change summary for alert #19075 (as of Wed, 30 Jan 2019 13:06:56 GMT) ==

Improvements:

37% raptor-tp6-docs-firefox raptor-tp6-docs-firefox-fcp osx-10-10 opt 2,135.67 -> 1,356.25
36% raptor-tp6-slides-firefox raptor-tp6-slides-firefox-fcp osx-10-10 opt 2,424.92 -> 1,543.17
32% raptor-tp6-docs-firefox osx-10-10 opt 2,453.17 -> 1,669.10
21% raptor-tp6-slides-firefox osx-10-10 opt 3,114.61 -> 2,475.20
15% raptor-tp6-docs-firefox windows10-64-qr opt 1,447.98 -> 1,232.27
15% raptor-tp6-docs-firefox windows10-64 opt 1,428.34 -> 1,220.77
13% raptor-tp6-docs-firefox windows10-64 pgo 1,342.19 -> 1,172.79
12% raptor-tp6-docs-firefox windows7-32 opt 1,349.17 -> 1,182.40
12% raptor-tp6-docs-firefox windows7-32 pgo 1,281.96 -> 1,128.11

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=19075

You need to log in before you can comment on or make changes to this bug.