1482032 - Only register a value for CONTENT_FRAME_TIME if we actually drew something

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Sotaro Ikeda [:sotaro]]

Created by Bug 1481950 Comment 1.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce forwarding EmptyTransaction if it is not necessary

Comment 2

[Sotaro Ikeda [:sotaro]]

With attachment 8998733 [details] [diff] [review], WrBridge()->SendSetFocusTarget() is still called. It could be handled as a different problem.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Reduce forwarding EmptyTransaction if it is not necessary

Comment 4

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba45a1744afb27f1c2523a2a2583948c1ea6e1bd

Comment 5

[Sotaro Ikeda [:sotaro]]

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

Comment 6

[Sotaro Ikeda [:sotaro]]

(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.

Comment 7

[Sotaro Ikeda [:sotaro]]

Changed summary by comment 6.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - A bit clean up of FlushTransactionIdsForEpoch()

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Only register a value for CONTENT_FRAME_TIME if we actually drew something

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Only register a value for CONTENT_FRAME_TIME if we actually drew something

Update a comment.

Comment 11

[Sotaro Ikeda [:sotaro]]

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?

Comment 12

[Matt Woodrow (:mattwoodrow)]

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?

Comment 13

[Sotaro Ikeda [:sotaro]]

(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%


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

Comment 14

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4945fee0ac919795e6c1b8c88fce54b92495e319

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c1f4543fd3fb
https://hg.mozilla.org/mozilla-central/rev/bc38e7f7ca73