Closed Bug 1307242 Opened 8 years ago Closed 8 years ago

Implement Time to non-blank Paint (TFP) for Telemetry/Profiler

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed

People

(Reporter: Harald, Assigned: mstange)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1307242 - Call nsPresContext::NotifyNonBlankPaint when building the display list for a presshell that contains more than just canvas backgrounds.
58 bytes, text/x-review-board-request
mattwoodrow
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1307242 - Add infrastructure for measuring time to non-blank paint.
58 bytes, text/x-review-board-request
bkelly
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1307242 - Add a non-blank paint telemetry probe for foreground root content documents. , f?bsmedberg
58 bytes, text/x-review-board-request
bkelly
: review+
lizzard
: approval-mozilla-beta+
Details
squashed patch for beta
20.36 KB, patch
lizzard
: approval-mozilla-beta-
lizzard
: approval-mozilla-release-
Details | Diff | Splinter Review 
"Perceived page load time is as important as the actual load time. Visible indication of progress directly improves user perception of speed. When the user gets timely feedback that “It is happening” they feel much better, and they perceive the site as faster." [1]

Timing should be exposed in SPS Profiler and Web Perf API.

[1]: https://docs.google.com/document/d/1wdxSXo_jctZjdPaJeTtYYFF-rLtUFxrU72_7h9qbQaM/edit#heading=h.xqk5pcolcpq1
Depends on: 1298380
No longer depends on: 1298381
Blocks: 1298380
No longer depends on: 1298380
Summary: Implement Time to non-blank Paint → Implement Time to non-blank Paint (TFP)
:jet, who would own the implementation?

This metric comes up repeatedly as the most important for perceived page load performance. It would be important to have for the roll out of 64bit on windows, so we can validate its usefulness and baseline it for future performance work.
Flags: needinfo?(bugs)
Clarified the minimal scope for this bug, which is only telemetry and profiler. Developer facing numbers depend on validation and spec stabilizing.
Keywords: dev-doc-needed
Summary: Implement Time to non-blank Paint (TFP) → Implement Time to non-blank Paint (TFP) for Telemetry/Profiler
Assignee: nobody → mstange
Markus will implement the startTime/stopTime markers in Gecko, and the related infrastructure to expose these in the Profiler.
Flags: needinfo?(bugs)
Meeting about it:

- While paint suppression is active this should not trigger first non-blank paint
- Text or image does not have to present, just anything other than background color
- Measure since navigation start
- Will be reference for other metrics, :digitarald to clarify
Attachment #8806515 - Attachment is obsolete: true
Attachment #8806517 - Flags: review?(bugs)
Attachment #8807278 - Flags: review?(bugs)
Attachment #8806517 - Flags: feedback?(benjamin)
Attachment #8807278 - Flags: feedback?(benjamin)
Attachment #8806517 - Flags: review?(bugs) → review?(bkelly)
Attachment #8807278 - Flags: review?(bugs) → review?(bkelly)
Comment on attachment 8806516 [details]
Bug 1307242 - Call nsPresContext::NotifyNonBlankPaint when building the display list for a presshell that contains more than just canvas backgrounds.

https://reviewboard.mozilla.org/r/89914/#review90230

::: layout/base/nsDisplayList.cpp:1038
(Diff revision 2)
> +{
> +  for (nsDisplayItem* i = aList->GetBottom(); i != nullptr; i = i->GetAbove()) {
> +    switch (i->GetType()) {
> +      case nsDisplayItem::TYPE_LAYER_EVENT_REGIONS:
> +      case nsDisplayItem::TYPE_CANVAS_BACKGROUND_COLOR:
> +      case nsDisplayItem::TYPE_CANVAS_BACKGROUND_IMAGE:

We could also have TYPE_SOLID_COLOR here, see PresShell::AddCanvasBackgroundColorItem

::: layout/base/nsDisplayList.cpp:1058
(Diff revision 2)
>  
> +  if (mIsPaintingToWindow) {
> +    nsPresContext* pc = aReferenceFrame->PresContext();
> +    if (!pc->HadNonBlankPaint()) {
> +      if (!CurrentPresShellState()->mIsBackgroundOnly &&
> +          DisplayListIsNonBlank(aContents)) {

What about the case where the page only contains a background image and nothing else?

It's not a particularly useful webpage, but it seems like it would get stuck and confuse things.

If we implement the other paint events (contentful/meaningful), we could make those imply non-blank.

::: layout/base/nsLayoutUtils.cpp:3187
(Diff revision 2)
>      builder.SetDescendIntoSubdocuments(false);
>    }
>  
>    builder.EnterPresShell(aFrame);
>    aFrame->BuildDisplayListForStackingContext(&builder, aRect, &list);
> -  builder.LeavePresShell(aFrame);
> +  builder.LeavePresShell(aFrame, &list);

We could just pass nullptr for the callers that are obviously not painting so that it's clearer that we aren't using the display list.
Comment on attachment 8806517 [details]
Bug 1307242 - Add infrastructure for measuring time to non-blank paint.

https://reviewboard.mozilla.org/r/89916/#review90482

Looks reasonable.  Mainly just some assertion suggestions to catch if code uses it improperly.

::: dom/base/nsDOMNavigationTiming.cpp:130
(Diff revision 2)
>  }
>  
>  void
> +nsDOMNavigationTiming::NotifyNonBlankPaint()
> +{
> +  mNonBlankPaintTimeStamp = TimeStamp::Now();

MOZ_ASSERT(NS_IsMainThread());
MOZ_ASSERT(mNonBlankPaintTimeStamp.IsNull());

We need to make sure we don't accidentally call this twice.

Also, do we want to MOZ_ASSERT(!mNavigationStartTimeStamp.IsNull())?  Otherwise this value will be kind of bogus.

::: dom/base/nsDOMNavigationTiming.cpp:143
(Diff revision 2)
> +    nsPrintfCString marker("Non-blank paint after %dms for URL %s",
> +                           int(elapsed.ToMilliseconds()), spec.get());
> +    PROFILER_MARKER(marker.get());
> +  }
> +
> +  Telemetry::AccumulateTimeDelta(Telemetry::TIME_TO_NON_BLANK_PAINT_ALL_MS,

Why not just use `DurationFromStart()` like everything else in this class and then use `Accumulate(static_cast<uint32>(value))`?  That is all telemetry is doing anyways:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#2835

::: layout/base/nsPresContext.cpp:2758
(Diff revision 2)
>  }
>  
> +void
> +nsPresContext::NotifyNonBlankPaint()
> +{
> +  mHadNonBlankPaint = true;

Again, should we assert this isn't double-called?

::: toolkit/components/telemetry/Histograms.json:10416
(Diff revision 2)
> +    "expires_in_version": "55",
> +    "kind": "exponential",
> +    "high": 100000,
> +    "n_buckets": 100,
> +    "bug_numbers": [1307242],
> +    "description": "The time between navigation start and the first non-blank paint of a document, in milliseconds. It measures all documents and subframes separately. If the page is loaded in the background or offscreen, the first non-blank paint might not happen until you switch to the tab or scroll to make the iframe visible. The non-blank paint timestamp is taken during display list building and does not include rasterization or compositing of that paint."

This needs a data steward review, right?
Attachment #8806517 - Flags: review?(bkelly) → review+
Comment on attachment 8806517 [details]
Bug 1307242 - Add infrastructure for measuring time to non-blank paint.

https://reviewboard.mozilla.org/r/89916/#review90482

> MOZ_ASSERT(NS_IsMainThread());
> MOZ_ASSERT(mNonBlankPaintTimeStamp.IsNull());
> 
> We need to make sure we don't accidentally call this twice.
> 
> Also, do we want to MOZ_ASSERT(!mNavigationStartTimeStamp.IsNull())?  Otherwise this value will be kind of bogus.

Good ideas. I will add all of these.

> Why not just use `DurationFromStart()` like everything else in this class and then use `Accumulate(static_cast<uint32>(value))`?  That is all telemetry is doing anyways:
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#2835

Mostly so that we reuse the exact same timestamp as the one we stored in mNonBlankPaintTimeStamp. (And the reason we store it in that field at all is that it'll be used as the zero reference point for other timings that we're going to add soon, like time to meaningful paint.)

I also had the idea of making telemetry insert profiler markers automatically. So AccumulateTimeDelta could add both a start and and end marker.

> Again, should we assert this isn't double-called?

Yeah, I will add an assert.

> This needs a data steward review, right?

Yes. This is why I requested feedback from bsmedberg.
Comment on attachment 8806516 [details]
Bug 1307242 - Call nsPresContext::NotifyNonBlankPaint when building the display list for a presshell that contains more than just canvas backgrounds.

https://reviewboard.mozilla.org/r/89914/#review90230

> We could also have TYPE_SOLID_COLOR here, see PresShell::AddCanvasBackgroundColorItem

Thanks. I will also add BACKGROUND and BACKGROUND_COLOR and check that the frame is a canvas frame, so that it's easier to remove the CANVAS_BACKGROUND types in the future.

> What about the case where the page only contains a background image and nothing else?
> 
> It's not a particularly useful webpage, but it seems like it would get stuck and confuse things.
> 
> If we implement the other paint events (contentful/meaningful), we could make those imply non-blank.

Not sure. For now we can treat them as blank. We can still change this later.

> We could just pass nullptr for the callers that are obviously not painting so that it's clearer that we aren't using the display list.

Hmm, do we want that to be obvious? I don't have a strong opinion on this, I'm happy to pass nullptr if you think that's better.
Comment on attachment 8807278 [details]
Bug 1307242 - Add a non-blank paint telemetry probe for foreground root content documents. , f?bsmedberg

https://reviewboard.mozilla.org/r/90494/#review90500

r=me with comments addressed.

::: docshell/base/nsDocShell.cpp:6184
(Diff revision 1)
> +    if (doc) {
> +      timing = doc->GetNavigationTiming();
> +    }
> +  }
> +  if (timing) {
> +    timing->NotifyDocShellActivenessChanged(aIsActive);

I don't really love this, but I guess its consistent with the existing code.

It would be nice if we could stop doing this work once FNBP occurs and we don't need the activity state any more.  A simple solution might be returning a boolean indicating if further activity updates are needed.

I can't help but think this method could be improved greatly by adding a nsDocShell `Listener` interface that different subsystems could use to get activity notifications.  That kind of refactor is probably a different bug, though.

::: dom/base/nsDOMNavigationTiming.cpp:222
(Diff revision 1)
> +
> +void
> +nsDOMNavigationTiming::NotifyDocShellActivenessChanged(bool aDocShellIsActive)
> +{
> +  if (!aDocShellIsActive) {
> +    mDocShellHasBeenActiveSinceNavigationStart = false;

This could be written as just:

`mDocShellHasBeenActiveSinceNavigationStart &&= aDocShellIsActive;`

::: image/SVGDocumentWrapper.cpp:365
(Diff revision 1)
>    //
>    // For a root document, DocShell would do these sort of things
>    // automatically. Since there is no DocShell for this wrapped SVG document,
>    // we must set it up manually.
>    RefPtr<nsDOMNavigationTiming> timing = new nsDOMNavigationTiming();
> -  timing->NotifyNavigationStart();
> +  timing->NotifyNavigationStart(false);

If you are going to use bare booleans like this at least add `false /* aIsActive */` comment.  Using an enum would be even better.
Attachment #8807278 - Flags: review?(bkelly) → review+
Comment on attachment 8806517 [details]
Bug 1307242 - Add infrastructure for measuring time to non-blank paint.

https://reviewboard.mozilla.org/r/89916/#review90862

::: toolkit/components/telemetry/Histograms.json:10416
(Diff revision 2)
> +    "expires_in_version": "55",
> +    "kind": "exponential",
> +    "high": 100000,
> +    "n_buckets": 100,
> +    "bug_numbers": [1307242],
> +    "description": "The time between navigation start and the first non-blank paint of a document, in milliseconds. It measures all documents and subframes separately. If the page is loaded in the background or offscreen, the first non-blank paint might not happen until you switch to the tab or scroll to make the iframe visible. The non-blank paint timestamp is taken during display list building and does not include rasterization or compositing of that paint."

Is this a monotonic interval? (It should be and usually telemetry timers are, but we should make sure).

I'm worried about the "page loaded in the background" subtlety here. Why is it valuable to include these numbers in this metric? Won't that just confuse the monitoring that we really care about, which is how long it takes front-page navigation to complete?

I'll follow up separately with Harald because I'm completely clear on how this is an essential metric for automated comparison with Chrome, but I don't understand how collecting it in telemetry will tell us much. And this is collecting from prerelease only.
Attachment #8806517 - Flags: review+
> I'm worried about the "page loaded in the background" subtlety here.

To be conservative we could remove the TIME_TO_NON_BLANK_PAINT_ALL_MS probe as it isn't something we are tracking for performance. It would have a lot of noise with tabs, iframes and such where platform delays painting until items are actually visible.

> I don't understand how collecting it in telemetry will tell us much

Short answer is better modeling for how changes we see in lab benchmarks later impact users in the field. Happy to follow up offline.
Comment on attachment 8807278 [details]
Bug 1307242 - Add a non-blank paint telemetry probe for foreground root content documents. , f?bsmedberg

https://reviewboard.mozilla.org/r/90494/#review90886

Oh I missed that there were two.
Attachment #8807278 - Flags: review+
Attachment #8806517 - Flags: feedback?(benjamin)
Attachment #8807278 - Flags: feedback?(benjamin)
> To be conservative we could remove the TIME_TO_NON_BLANK_PAINT_ALL_MS probe as it isn't something we are tracking for performance. It would have a lot of noise with tabs, iframes and such where platform delays painting until items are actually visible.

After checking with :mstange, I want to make this more clear that we should remove the _ALL_ probe as it is not what we want to track for perception and too noisy to be a diagnostic probe.
Comment on attachment 8806516 [details]
Bug 1307242 - Call nsPresContext::NotifyNonBlankPaint when building the display list for a presshell that contains more than just canvas backgrounds.

https://reviewboard.mozilla.org/r/89914/#review90230

> Hmm, do we want that to be obvious? I don't have a strong opinion on this, I'm happy to pass nullptr if you think that's better.

I guess we could also have LeavePresShellForPainting() or something that takes the display list, and have that call LeavePresShell as well as dealing with the display list.

I don't really care much, do whatever you think is best. Having the parameter passed at the other call sites seems to imply that we are going to do something with it though.
Comment on attachment 8806516 [details]
Bug 1307242 - Call nsPresContext::NotifyNonBlankPaint when building the display list for a presshell that contains more than just canvas backgrounds.

https://reviewboard.mozilla.org/r/89914/#review92098
Attachment #8806516 - Flags: review?(matt.woodrow) → review+
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0318d13a32f9
Add infrastructure for measuring time to non-blank paint. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/10dd4c02d4b6
Call nsPresContext::NotifyNonBlankPaint when building the display list for a presshell that contains more than just canvas backgrounds. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa04cab9513b
Add a non-blank paint telemetry probe for foreground root content documents. r=bkelly, f=bsmedberg
Related, Chrome's Intent to Implement to expose this timestamp: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/kyll3WFKTig
Comment on attachment 8806517 [details]
Bug 1307242 - Add infrastructure for measuring time to non-blank paint.

Approval Request Comment
[Feature/Bug causing the regression]:

Adds a new probe to measure the perceived page load milestone of first non-blank paint. The value is reported via telemetry and as marker in platform profiles.

[User impact if declined]:

None.

[Has the fix been verified in Nightly?]:

Telemetry data was collected and reviewed for Nightly and Aurora.

[List of other uplifts needed for the feature/fix]:

All attachments of bug 1307242.

[Is the change risky?]:

Very low risk.

[Why is the change risky/not risky?]:

Just adds a probe that validates the state of painting and adds a marker to the profiler.

[String changes made/needed]:

None
Attachment #8806517 - Flags: approval-mozilla-beta?
Attachment #8807278 - Flags: approval-mozilla-beta?
Attachment #8806516 - Flags: approval-mozilla-beta?
Comment on attachment 8806517 [details]
Bug 1307242 - Add infrastructure for measuring time to non-blank paint.

Adds telemetry probe, already in aurora for a month so let's try this for beta 13.
Attachment #8806517 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8806516 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8807278 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(mstange)

    
    

    
  
We've already spun the first release candidate for 51. Comment 32 says this is low-risk, but I'd still like to have RelMan sign off on landing it this late in the cycle.
Flags: needinfo?(lhenry)

    
    

    
  
I think we should let this ride with 52. I don't want this to possibly delay shipping 51 -- we are in the last few days before the release.
Flags: needinfo?(lhenry)

    
    

    
  
Comment on attachment 8827965 [details] [diff] [review]
squashed patch for beta





Too late for beta (we are about to start the RC2 build for next week's release)

        Attachment #8827965 -
        Flags: approval-mozilla-release-

        Attachment #8827965 -
        Flags: approval-mozilla-beta-
Depends on: 1347061

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: