Spec: https://w3c.github.io/paint-timing/ FCP gives web developers an indication of when painting was actually performed and when it's contentful (contains text/canvas/image/svg). This is part of the painting API, and a part where WebKit is more simiilar to other browsers so is less ambiguous than the other entry of paint-API, first-paint
See my comment on webkit.org/78011. Before implementing this API, we should adjust our heuristics for the first meaningful paint.
(In reply to Ryosuke Niwa from comment #1) > See my comment on webkit.org/78011. Before implementing this API, we should > adjust our heuristics for the first meaningful paint. Great! I created a bug to track the change in heuristic: https://bugs.webkit.org/show_bug.cgi?id=208500
Created attachment 394078 [details] Patch
I made a crack at the API adhering to the current version of the spec. I know it's not currently adjusted to VNE heuristic, for the following reasons: - It's behind a flag, we can adjust VNE/FCP as we go along, and also as the spec evolves, before enabling it in Safari etc. - in many cases VNE would happen before FCP, as VNE (unlike FCP) is bound to happen after all resources load, and unlike FCP, VNE is not layout bound. - VNE currently has no tests. The new FCP tests from W3C could serve as infrastructure for VNE tests. - I wanted to move forward, and it's been difficult to contribute to VNE as it's not something that has a spec. It currently covers 23/24 of the W3C tests, and more could be added incrementally. If we still want to adjust VNE before moving forward with it it's of course OK, but I'd be happy to get some guidance on how to do that (See https://bugs.webkit.org/show_bug.cgi?id=208501 for example, the discussion is a bit opaque).
I don't think we ever want to FCP that's totally different from VNE since then FCP would become a fictional metric.
Comment on attachment 394078 [details] Patch r- on that basis
(In reply to Ryosuke Niwa from comment #5) > I don't think we ever want to FCP that's totally different from VNE since > then FCP would become a fictional metric. Oops, I think I've used wrong wording to describe this work! FCP here is not totally different from VNE - what I meant is that it doesn't count directly on VNE code because VNE is a pre-layout heuristic metric and FCP is a post-layout accurate metric. For example, the characters of text that has an empty bounding rect would count for VNE but not for FCP, because the bounding box can only be calculated after layout. It's not a fictional metric in this patch - it reports the first contentful paint as defined in the spec. Since VNE delays paint, that delay is indirectly taken into account, but not directly in the code path. If this is still unclear I'd love to chat about this on Slack :)
Created attachment 394280 [details] Patch
A new patch. explicity referring to VNE.
Created attachment 394284 [details] Patch
Created attachment 394370 [details] Patch
Created attachment 394899 [details] Patch
Created attachment 394904 [details] Patch
Created attachment 394907 [details] Patch
Created attachment 394909 [details] Patch
Created attachment 394911 [details] Patch
Created attachment 394913 [details] Patch
Created attachment 394917 [details] Patch
Created attachment 394919 [details] Patch
Created attachment 394942 [details] Patch
OK, the build failure is same as https://bugs.webkit.org/show_bug.cgi?id=201641 Hope we get the WIN bots to run a clean build, they currently can't handle adding preference keys.
Comment on attachment 394942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394942&action=review > Source/WebCore/page/FrameView.cpp:4202 > + || frame().document()->securityOrigin().canAccess(frame().mainFrame().document()->securityOrigin()); Weird indentation. I'm surprised we don't have a helper on Frame for this. > Source/WebCore/page/FrameView.cpp:4205 > +void FrameView::markPaintTimingIfNeeded() Does this really belong here, or on Document? Does FCP fire when a page comes out of the page cache? How does it relate to the page visibility API? > Source/WebCore/page/FrameView.h:828 > + bool m_didMarkFirstContentfulPaint { false }; Please don't put this member variable here before all the others. For one thing, you've introduced 3 bytes of padding before m_frame. Group it with other bools lower down. > Source/WebCore/page/PerformancePaintTiming.h:35 > + static Ref<PerformancePaintTiming> createFirstContentfulPaint(double timeStamp) Should that be a DOMHighResTimeStamp or a MonotonicTime? We try to avoid raw 'doubles' because they are ambiguous. > Source/WebCore/rendering/RenderElement.cpp:334 > + if (!opacity() || isSVGHiddenContainer()) We don't care about opacity on ancestors? > Source/WebCore/rendering/RenderElement.cpp:338 > + if (style().visibility() != Visibility::Visible) We don't care about visibility on ancestors? > Source/WebCore/rendering/RenderElement.cpp:344 > + return current->boundingClientRect().intersects(documentRect); Are we only calling this when layout is up-to-date, so that boundingClientRect() gives a valid answer? Seems odd to bounce to Element to call boundingClientRect() which is going to call right back into rendering code. > Source/WebCore/rendering/RenderElement.cpp:355 > + for (const RenderObject* renderObject = firstChild(); renderObject; renderObject = renderObject->nextSibling()) { > + if (renderObject->containsPaintableContent()) > + return true; > + } It would be much nicer to hoist the recursion out of the function that does the actual work. This should use render tree traversal helpers, and it should be really obvious that this is a massive, expensive full render tree traversal, with boundingClientRect() (an ancestor tree walk) for each renderer. In fact, I think we need to do this more efficiently, and be able to clip out non-visible areas, which makes it more like paint traversal. I'm going to r- because I think the perf implications here are a showstopper.
Comment on attachment 394942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394942&action=review >> Source/WebCore/rendering/RenderElement.cpp:355 >> + } > > It would be much nicer to hoist the recursion out of the function that does the actual work. This should use render tree traversal helpers, and it should be really obvious that this is a massive, expensive full render tree traversal, with boundingClientRect() (an ancestor tree walk) for each renderer. > > In fact, I think we need to do this more efficiently, and be able to clip out non-visible areas, which makes it more like paint traversal. I'm going to r- because I think the perf implications here are a showstopper. Actually this function should not even be on the RenderElement (the only non-recursive entry point is the RenderView). It looks more like a helper function that collects "paintable information" about the render tree.
Comment on attachment 394942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394942&action=review >> Source/WebCore/page/FrameView.cpp:4202 >> + || frame().document()->securityOrigin().canAccess(frame().mainFrame().document()->securityOrigin()); > > Weird indentation. I'm surprised we don't have a helper on Frame for this. Yea, actually check-webkit-style gave me trouble when I tried to do anything different. Will find a way. >> Source/WebCore/page/FrameView.cpp:4205 >> +void FrameView::markPaintTimingIfNeeded() > > Does this really belong here, or on Document? > > Does FCP fire when a page comes out of the page cache? How does it relate to the page visibility API? It can be in Document, maybe that's better. By the spec, if the page comes out of the page cache before FCP was called, it might still be called. The mark of whether it was called or not is associated with Document. >> Source/WebCore/page/FrameView.h:828 >> + bool m_didMarkFirstContentfulPaint { false }; > > Please don't put this member variable here before all the others. For one thing, you've introduced 3 bytes of padding before m_frame. Group it with other bools lower down. Sure. I think it needs to move to Document. though as per previous comment >> Source/WebCore/page/PerformancePaintTiming.h:35 >> + static Ref<PerformancePaintTiming> createFirstContentfulPaint(double timeStamp) > > Should that be a DOMHighResTimeStamp or a MonotonicTime? We try to avoid raw 'doubles' because they are ambiguous. Aye. >> Source/WebCore/rendering/RenderElement.cpp:334 >> + if (!opacity() || isSVGHiddenContainer()) > > We don't care about opacity on ancestors? No, because this is called recursively, and this early return exits before we check descendant opacity. >> Source/WebCore/rendering/RenderElement.cpp:338 >> + if (style().visibility() != Visibility::Visible) > > We don't care about visibility on ancestors? Isn't visibility inherited rather than affected by ancestors? >> Source/WebCore/rendering/RenderElement.cpp:344 >> + return current->boundingClientRect().intersects(documentRect); > > Are we only calling this when layout is up-to-date, so that boundingClientRect() gives a valid answer? > Seems odd to bounce to Element to call boundingClientRect() which is going to call right back into rendering code. Yes - we are only calling this when layout is up to date. I'll add an assert for that. Maybe boundingClientRect can move into here, and Element would call it? >>> Source/WebCore/rendering/RenderElement.cpp:355 >>> + } >> >> It would be much nicer to hoist the recursion out of the function that does the actual work. This should use render tree traversal helpers, and it should be really obvious that this is a massive, expensive full render tree traversal, with boundingClientRect() (an ancestor tree walk) for each renderer. >> >> In fact, I think we need to do this more efficiently, and be able to clip out non-visible areas, which makes it more like paint traversal. I'm going to r- because I think the perf implications here are a showstopper. > > Actually this function should not even be on the RenderElement (the only non-recursive entry point is the RenderView). It looks more like a helper function that collects "paintable information" about the render tree. @smfr: Regarding the performance hit - note that this doesn't called during render etc - only in the beginning when FCP has not been reported yet. The performance hit would only be there in edge cases where there is a very deep tree of non-paintable things. But I'll try to think of alternatives. @zalan: sure, will revise.
Created attachment 395353 [details] Patch
Created attachment 395354 [details] Patch
Revised based on comments from @zalan and @smfr: - Moved paintability checks to PaintTimingRenderSupport (other name suggestions welcome) - Moved eligibility logic to Document, made more succinct. - Used more efficient methods for recursion and geometry checks - Added comments and asserts to make sure the expensive path would only very rarely be used.
Created attachment 395549 [details] Patch
Created attachment 396106 [details] Patch
Added a test that makes sure VNE blocks FCP - a delayed stylesheet loading would take place before FCP.
Created attachment 396308 [details] Patch
Comment on attachment 396308 [details] Patch The rendering bits look good. Great test coverage!
Comment on attachment 396308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396308&action=review > Source/WebCore/dom/Document.cpp:3152 > +void Document::markPaintTimingIfNeeded() Would it be accurate to call this reportPaintTimingIfNeeded? I'm not really a fan of the "mark" terminology. > Source/WebCore/page/FrameView.cpp:985 > + if (auto* document = frame().document()) > + document->markPaintTimingIfNeeded(); This is the wrong place for this.It should go in Page::updateRendering(). > Source/WebCore/page/PerformanceEntry.h:55 > Mark = 1 << 1, > Measure = 1 << 2, > Resource = 1 << 3, > + Paint = 1 << 4 I like these lined up (adjust spaces before the =) > Source/WebCore/rendering/ContentfulPaintChecker.cpp:40 > +static bool intersectsWithDocumentRect(const RenderObject& object, const IntRect& documentRect) intersectsDocumentRect() > Source/WebCore/rendering/ContentfulPaintChecker.cpp:47 > + if (textRenderer.localToAbsoluteQuad(FloatQuad(box.rect())).boundingBox().intersects(documentRect)) This localToAbsolute() is another ancestor tree walk. > Source/WebCore/rendering/ContentfulPaintChecker.cpp:109 > + for (auto& ancestor : lineageOfType<RenderObject>(object)) { > + if (!ancestor.style().opacity()) > + return false; > + } Ouch. For each renderer, you're doing an ancestor tree walk. Starts getting > O(n) > Source/WebCore/rendering/ContentfulPaintChecker.cpp:114 > +bool qualifiesForContentfulPaint(RenderView& view) Might be nicer to pass a FrameView here and assert !frameView.needsLayout(). > Source/WebCore/rendering/ContentfulPaintChecker.cpp:120 > + for (auto& object : descendantsOfType<RenderObject>(view)) { > + if (isContentful(object) && isPaintable(object, documentRect)) > + return true; > + } Potentially a full tree walk, which we need to avoid. Why doesn't this make use of didMarkVisuallyNonEmptyPixels() which you're adding in the other patch? If we know which renderers have been marked, can't we remember them and ask them directly? > Source/WebCore/rendering/ContentfulPaintChecker.h:27 > +bool qualifiesForContentfulPaint(RenderView&); Maybe make this a class with static member functions. I suspect we'll want a real controller object with more state one day.
Comment on attachment 396308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396308&action=review >> Source/WebCore/dom/Document.cpp:3152 >> +void Document::markPaintTimingIfNeeded() > > Would it be accurate to call this reportPaintTimingIfNeeded? I'm not really a fan of the "mark" terminology. Sure, I tried to follow the same language as the spec. >> Source/WebCore/page/FrameView.cpp:985 >> + document->markPaintTimingIfNeeded(); > > This is the wrong place for this.It should go in Page::updateRendering(). Gotcha. >> Source/WebCore/page/PerformanceEntry.h:55 >> + Paint = 1 << 4 > > I like these lined up (adjust spaces before the =) ok >> Source/WebCore/rendering/ContentfulPaintChecker.cpp:47 >> + if (textRenderer.localToAbsoluteQuad(FloatQuad(box.rect())).boundingBox().intersects(documentRect)) > > This localToAbsolute() is another ancestor tree walk. I guess an alternative would be to pass the matrix going down the tree, using a child-walk rather than ancestor walk, or to save the potentially-contentful elements somewhere. >> Source/WebCore/rendering/ContentfulPaintChecker.cpp:109 >> + } > > Ouch. For each renderer, you're doing an ancestor tree walk. Starts getting > O(n) The other option is to do a child walk and stop when I get to zero opacity, like I did in a previous patch. would that work better? >> Source/WebCore/rendering/ContentfulPaintChecker.cpp:114 >> +bool qualifiesForContentfulPaint(RenderView& view) > > Might be nicer to pass a FrameView here and assert !frameView.needsLayout(). ok >> Source/WebCore/rendering/ContentfulPaintChecker.cpp:120 >> + } > > Potentially a full tree walk, which we need to avoid. > > Why doesn't this make use of didMarkVisuallyNonEmptyPixels() which you're adding in the other patch? If we know which renderers have been marked, can't we remember them and ask them directly? We can do that, but then we would need to save a set of them somewhere (in FrameView?) Would that be preferrable? I guess we can clear that set once we report paint timing. >> Source/WebCore/rendering/ContentfulPaintChecker.h:27 >> +bool qualifiesForContentfulPaint(RenderView&); > > Maybe make this a class with static member functions. I suspect we'll want a real controller object with more state one day. sure
Created attachment 396515 [details] Patch
(In reply to Noam Rosenthal from comment #35) > Created attachment 396515 [details] > Patch Riding on existing walker determineNonLayerDescendantsPaintedContent, which is capped at 200... so FCP will be reported if we've passed 200 non-layer elements and still not sure if we're contentful or not.
Created attachment 396634 [details] Patch
Created attachment 396645 [details] Patch
Created attachment 396651 [details] Patch
Comment on attachment 396651 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396651&action=review > Source/WebCore/page/PerformancePaintTiming.h:33 > +class PerformancePaintTiming final : public PerformanceEntry { I would have called this PerformancePaintTimingEntry but I guess the web API precludes this. > Source/WebCore/rendering/ContentfulPaintChecker.cpp:44 > + TransformationMatrix transform = parentTransform * layer.currentTransform(); This manual transform math is almost certainly wrong, and I've no idea how you can test it to make sure it's correct here. Think about preserves3D, flattening, perspective....
Comment on attachment 396651 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396651&action=review >> Source/WebCore/rendering/ContentfulPaintChecker.cpp:44 >> + TransformationMatrix transform = parentTransform * layer.currentTransform(); > > This manual transform math is almost certainly wrong, and I've no idea how you can test it to make sure it's correct here. Think about preserves3D, flattening, perspective.... any suggestions on where to get this right without doing another ancestor walk? All I'm really looking for is to see that the layer is not transformed to nothing (e.g. by a 3d rotate) or to some place outside the document rect (e.g. a negative y).
Created attachment 397073 [details] Patch
New patch uses fake-paint mechanism instead of custom tree walking.
Created attachment 397077 [details] Patch
Created attachment 397087 [details] Patch: using fake paint
Created attachment 397088 [details] Patch: using fake paint
Created attachment 397330 [details] Patch
Created attachment 397716 [details] Patch
Created attachment 397730 [details] Patch
Created attachment 397802 [details] Patch
Created attachment 397808 [details] Patch
Comment on attachment 397808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397808&action=review > Source/WebCore/dom/Document.cpp:3155 > +void Document::reportPaintTimingIfNeeded() I'd prefer this not be called "report". It doesn't synchronously call out to JS (which is good); it enqueues a perf entry, which is dispatched via the task queue (a zero-delay timer). > Source/WebCore/dom/Document.h:2050 > + bool m_didReportFirstContentfulPaint { false }; didEnqueue? > Source/WebCore/page/Page.cpp:1399 > + document.reportPaintTimingIfNeeded(); This is where "report" terminology is bad. It should be clear that this function does not trigger script synchronously. > Source/WebCore/platform/graphics/GraphicsContext.h:519 > + void setContentfulPaintDetected() { m_contenfulPaintDetected = true; } > + bool contenfulPaintDetected() const { return m_contenfulPaintDetected; } We should probably change PaintInvalidationReasons to be less about invalidation, and more general. Then this could just be a value of PaintInvalidationReasons. Same for event region painting. Not necessary for this patch though. > Source/WebCore/rendering/TextPainter.cpp:111 > + if (!textRun.text().toStringWithoutCopying().isAllSpecialCharacters<isHTMLSpace>()) Not sure you need the toStringWithoutCopying() there. > Source/WebKit/Shared/WebPreferences.yaml:833 > +PaintTimingEnabled: > + type: bool > + defaultValue: false > + humanReadableName: "Paint Timing" > + humanReadableDescription: "Enable PaintTiming API" > + webcoreBinding: RuntimeEnabledFeatures This should be "category: experimental" right? > Tools/WebKitTestRunner/TestController.cpp:939 > + WKPreferencesSetPaintTimingEnabled(preferences, true); WTR turns on all experimental features by default. Don't think you need this. > LayoutTests/platform/mac-wk1/TestExpectations:567 > +imported/w3c/web-platform-tests/paint-timing [ Skip ] > +performance-api/paint-timing-apis.html [ Skip ] > +performance-api/paint-timing-frames.html [ Skip ] > +performance-api/paint-timing-with-worker.html [ Skip ] > +http/tests/performance/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars.html [ Skip ] > +http/tests/performance/performance-paint-timing-fcp-after-visually-non-empty-for-style.html [ Skip ] > +performance-api/performance-observer-first-contentful-paint.html [ Skip ] Please sort. Can we make performance-api/paint-timing/ and just skip the whole dir?
Comment on attachment 397808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397808&action=review >> Source/WebCore/dom/Document.cpp:3155 >> +void Document::reportPaintTimingIfNeeded() > > I'd prefer this not be called "report". It doesn't synchronously call out to JS (which is good); it enqueues a perf entry, which is dispatched via the task queue (a zero-delay timer). enqueuePerformanceTimingEntryIfNeeded()? >> Source/WebCore/page/Page.cpp:1399 >> + document.reportPaintTimingIfNeeded(); > > This is where "report" terminology is bad. It should be clear that this function does not trigger script synchronously. Got it. changing to enqueuePaintTimingEntryIfNeeded() >> Source/WebCore/platform/graphics/GraphicsContext.h:519 >> + bool contenfulPaintDetected() const { return m_contenfulPaintDetected; } > > We should probably change PaintInvalidationReasons to be less about invalidation, and more general. Then this could just be a value of PaintInvalidationReasons. Same for event region painting. > > Not necessary for this patch though. Yea, I found the "invalidation" terminology a bit misleading. will see if I can do a followup about it >> Source/WebCore/rendering/TextPainter.cpp:111 >> + if (!textRun.text().toStringWithoutCopying().isAllSpecialCharacters<isHTMLSpace>()) > > Not sure you need the toStringWithoutCopying() there. Regular toString() is ok here then? >> Source/WebKit/Shared/WebPreferences.yaml:833 >> + webcoreBinding: RuntimeEnabledFeatures > > This should be "category: experimental" right? right. >> Tools/WebKitTestRunner/TestController.cpp:939 >> + WKPreferencesSetPaintTimingEnabled(preferences, true); > > WTR turns on all experimental features by default. Don't think you need this. ok >> LayoutTests/platform/mac-wk1/TestExpectations:567 >> +performance-api/performance-observer-first-contentful-paint.html [ Skip ] > > Please sort. > > Can we make performance-api/paint-timing/ and just skip the whole dir? ok
Created attachment 397880 [details] Patch for landing
> > >> Source/WebCore/rendering/TextPainter.cpp:111 > >> + if (!textRun.text().toStringWithoutCopying().isAllSpecialCharacters<isHTMLSpace>()) > > > > Not sure you need the toStringWithoutCopying() there. > > Regular toString() is ok here then? In other places we just do textRun.text().isAllSpecialCharacters()
Created attachment 397886 [details] Patch for landing
Committed r260851: <https://trac.webkit.org/changeset/260851> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397886 [details].
<rdar://problem/62551526>
The new test http/tests/performance/paint-timing/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars.html Added in https://trac.webkit.org/changeset/260851/webkit is a flaky failure on Mac History: https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fperformance%2Fpaint-timing%2Fperformance-paint-timing-fcp-after-visually-non-empty-for-num-chars.html Diff: --- /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/performance/paint-timing/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars-expected.txt +++ /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/http/tests/performance/paint-timing/performance-paint-timing-fcp-after-visually-non-empty-for-num-chars-actual.txt @@ -1,3 +1,3 @@ -PASS paintEntry.startTime > timestamps[expectedIterations] is true +FAIL paintEntry.startTime > timestamps[expectedIterations] should be true. Was false. PASS paintEntry.startTime < timestamps[expectedIterations + 2] is true
(In reply to Truitt Savell from comment #59) > The new test > http/tests/performance/paint-timing/performance-paint-timing-fcp-after- > visually-non-empty-for-num-chars.html > > Added in https://trac.webkit.org/changeset/260851/webkit > > is a flaky failure on Mac > > History: > https://results.webkit.org/?suite=layout- > tests&test=http%2Ftests%2Fperformance%2Fpaint-timing%2Fperformance-paint- > timing-fcp-after-visually-non-empty-for-num-chars.html > > Diff: > --- > /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/ > http/tests/performance/paint-timing/performance-paint-timing-fcp-after- > visually-non-empty-for-num-chars-expected.txt > +++ > /Volumes/Data/slave/catalina-release-tests-wk2/build/layout-test-results/ > http/tests/performance/paint-timing/performance-paint-timing-fcp-after- > visually-non-empty-for-num-chars-actual.txt > @@ -1,3 +1,3 @@ > -PASS paintEntry.startTime > timestamps[expectedIterations] is true > +FAIL paintEntry.startTime > timestamps[expectedIterations] should be true. > Was false. > PASS paintEntry.startTime < timestamps[expectedIterations + 2] is true It's OK to mark it as flaky for now, and I'll work on finding a solution for it. VNE tests are a bit difficult... I can prepare a TextExpectations patch tomorrow if someone doesn't beat me to it.