Closed Bug 864829 Opened 7 years ago Closed 6 years ago

Add graphics performance warning feature

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: BenWa, Assigned: BenWa)

Details

Attachments

(1 file, 1 obsolete file)

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?
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.
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).
for comments*
(In reply to Benoit Girard (:BenWa) from comment #3)
> for comments*

Oh, good, I was worried there for a second.
This would be excellent to have.
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8419024 - Flags: review?(milan)
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 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.
Attachment #8419024 - Flags: review?(milan)
Attachment #8419024 - Flags: review?(bjacob)
Attachment #8419024 - Flags: feedback+
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.
Attachment #8419024 - Flags: review?(bjacob) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/df8c4d6994c6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.