Only register a value for CONTENT_FRAME_TIME if we actually drew something

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

(Blocks 2 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

11 months ago
Created by Bug 1481950 Comment 1.
Assignee

Updated

11 months ago
Assignee: nobody → sotaro.ikeda.g
Assignee

Updated

11 months ago
Blocks: 1481950
Comment hidden (obsolete)
Assignee

Comment 2

11 months ago
With attachment 8998733 [details] [diff] [review], WrBridge()->SendSetFocusTarget() is still called. It could be handled as a different problem.
Comment hidden (obsolete)
Assignee

Comment 5

11 months ago
By Bug 1481950 comment 3, attachment 8998734 [details] [diff] [review] might affect to MozAfterPaint.
Assignee

Comment 6

11 months ago
(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> By Bug 1481950 comment 3, attachment 8998734 [details] [diff] [review] might
> affect to MozAfterPaint.

Copy of Bug 1481950 comment 3.

---------------------------------------------

Thanks Sotaro!

It might also make sense to only register a value for CONTENT_FRAME_TIME if we actually drew something. Both backends seem to have fallbacks path where we call DidComposite/NotifyPipelineRendered even when no work was done.

I think we need some code to run to ensure MozAfterPaint is delivered (since JS can be waiting for it), but we should be able to skip recording the telemetry value.
Assignee

Updated

11 months ago
Attachment #8998734 - Attachment is obsolete: true
Assignee

Updated

11 months ago
Summary: Reduce forwarding EmptyTransaction if it is not necessary → Only register a value for CONTENT_FRAME_TIME if we actually drew something
Assignee

Comment 7

11 months ago
Changed summary by comment 6.
Comment hidden (obsolete)
Assignee

Comment 11

11 months ago
Comment on attachment 8999506 [details] [diff] [review]
patch part 2 - Only register a value for CONTENT_FRAME_TIME if we actually drew something

:mattwoodrow, can you feedback to the patch?
Attachment #8999506 - Flags: review?(matt.woodrow)
Assignee

Updated

11 months ago
Attachment #8999506 - Flags: review?(matt.woodrow) → feedback?(matt.woodrow)
Comment on attachment 8999506 [details] [diff] [review]
patch part 2 - Only register a value for CONTENT_FRAME_TIME if we actually drew something

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

Looks good!

Have you checked in about:telemetry to see if the low values go away?
Attachment #8999506 - Flags: feedback?(matt.woodrow) → feedback+
Assignee

Comment 13

10 months ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> 
> Have you checked in about:telemetry to see if the low values go away?

Yes, I checked the CONTENT_FRAME_TIME in about:telemetry. The very low values went away.


Before applying the patches, CONTENT_FRAME_TIME was like the following.

****************************************
CONTENT_FRAME_TIME
159 samples, average = 107.3, sum = 17068

  2 |  0  0%
  3 |#  3  2%
  4 |##  6  4%
  5 |#  4  3%
  9 |  1  1%
 11 |  1  1%
 13 |#  4  3%
 15 |#  3  2%
 21 |  1  1%
 40 |  1  1%
 55 |#  2  1%
 64 |#  3  2%
 75 |#  3  2%
 88 |#  3  2%
103 |#########################  73  46%
120 |##########  29  18%
140 |###  9  6%
164 |##  5  3%
192 |#  4  3%
224 |#  3  2%
357 |  1  1%
417 |  0  0%

****************************************


After applying the patches, CONTENT_FRAME_TIME was like the following. The very low values went away.

****************************************
CONTENT_FRAME_TIME
187 samples, average = 122.3, sum = 22868

  9 |  0  0%
 11 |  1  1%
 34 |  1  1%
 64 |#  5  3%
 75 |#  4  2%
 88 |###  10  5%
103 |#########################  100  53%
120 |###########  43  23%
140 |##  8  4%
164 |##  6  3%
192 |#  4  2%
224 |#  3  2%
262 |  1  1%
417 |  1  1%
487 |  0  0%


****************************************
Assignee

Updated

10 months ago
Attachment #8999504 - Flags: review?(matt.woodrow)
Assignee

Updated

10 months ago
Attachment #8999506 - Flags: review?(matt.woodrow)
Attachment #8999504 - Flags: review?(matt.woodrow) → review+
Attachment #8999506 - Flags: review?(matt.woodrow) → review+

Comment 15

10 months ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f4543fd3fb
part 1 - A bit clean up of FlushTransactionIdsForEpoch() r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc38e7f7ca73
part 2 - Only register a value for CONTENT_FRAME_TIME if we actually drew something r=mattwoodrow

Comment 16

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c1f4543fd3fb
https://hg.mozilla.org/mozilla-central/rev/bc38e7f7ca73
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.