864829 - Add graphics performance warning feature

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

In a meeting we discussed a feature where we could log when we're hitting slow paths. Here's something a proposal where we can get a lot of bang for our time:
1) Add a pref, say 'gecko.perf.warnings'
2) Create GeckoPerfWarning.h with gecko_perf_warning(string module, enum level, string desc)

If we add this inside slow path we're adding a cheap well predicted branch.

What do you guys think?

Comment 1

[Nick Cameron [:nrc]]

I think this is an awesome idea - we already have this for OMTA and it was pretty useful. One thing that is useful is not just saying that we're going the slow way, but why.

Comment 2

[Benoit Girard (:BenWa)]

This has been quiet so far. I'll wait for ~48 hours for command before implementing this. I'm just going to log to stdout when active for now. We can log this to more places in follow bugs (such as within perf profiles/devtools).

Comment 3

[Benoit Girard (:BenWa)]

for comments*

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benoit Girard (:BenWa) from comment #3)
> for comments*

Oh, good, I was worried there for a second.

Comment 5

[Kelsey Gilbert [:jgilbert]]

This would be excellent to have.

Comment 6

[Benoit Girard (:BenWa)]

Here's an example:
http://people.mozilla.org/~bgirard/cleopatra/#report=daa59e8d16a8462ac0ae4e1d2c5e718ec5798eb7

1) Select the 'Logging' tab, mouse over the performance warning and it will show '| Log' in the timeline. You can see which composite (in this case all) uses an intermediate surface.

In this case it looks like we can't render layer tree with intermediate surfaces at 60 FPS even on a powerful MBP at 1080p. We could probably do better.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 8419024 [details] [diff] [review]
patch

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

::: gfx/thebes/gfxPlatform.h
@@ +57,5 @@
>  }
>  }
>  }
>  
> +#define MOZ_PERFORMANCE_WARNING(module, ...) \

I left a module field so that later we could easily chose to filter, for now it's not used.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8419024 [details] [diff] [review]
patch

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

I like that you used graphics independent name, with the "gfx" module name.  Let's us generalize the macro usage and perhaps eventually move it out of gfxPlatform because it's used "everywhere".  So, f+, and asked a peer to review.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8419024 [details] [diff] [review]
patch

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

R+ with this nit:

::: gfx/thebes/gfxPlatform.h
@@ +59,5 @@
>  }
>  
> +#define MOZ_PERFORMANCE_WARNING(module, ...) \
> +  if (gfxPrefs::PerfWarnings()) { \
> +    printf_stderr(__VA_ARGS__); \

Please write something like:

printf_stderr("[" module "] " __VA_ARGS__);

So that users have to pass a literal string for module and for the second parameter, and have to see the effect of the module parameter. This will prevent having people not know what they should pass for module, and will catch at compile time bugs where they would forget to pass it, like MOZ_PERFORMANCE_WARNING("message %d", param), and their formatted string would be passed as the module parameter.

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Milan Sreckovic [:milan] from comment #9)
> perhaps eventually move it out of
> gfxPlatform because it's used "everywhere".

Right, I first look in something like MFBT or the likes but decided to put it in GFX for now. I agree we should move it eventually.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch r=bjacob

https://hg.mozilla.org/integration/mozilla-inbound/rev/df8c4d6994c6

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df8c4d6994c6