Last Comment Bug 779940 - Add a layers transactions meter
: Add a layers transactions meter
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Matt Woodrow (:mattwoodrow)
:
Mentors:
Depends on:
Blocks: 780074
  Show dependency treegraph
 
Reported: 2012-08-02 11:20 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-08-29 07:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Meter layers transactions in addition to composites (10.38 KB, patch)
2012-08-02 16:40 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
cjones.bugs: review+
Details | Diff | Splinter Review
Meter layers transactions in addition to composites (10.89 KB, patch)
2012-08-06 19:52 PDT, Matt Woodrow (:mattwoodrow)
matt.woodrow: review+
Details | Diff | Splinter Review
Rebased, with added null check (11.03 KB, patch)
2012-08-25 19:15 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-02 11:20:29 PDT
We've had some bugs where we were sending way too many layers transactions, but the throttling of composition was hiding those bugs.  Matt added a second rate counter for layers transactions, in addition to the compositor counter.  I'd like to land that.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-02 16:40:17 PDT
Created attachment 648557 [details] [diff] [review]
Meter layers transactions in addition to composites
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-02 23:25:26 PDT
Comment on attachment 648557 [details] [diff] [review]
Meter layers transactions in addition to composites

>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h

>+  virtual void NotifyContentUpdated() {}

More precisely: |NotifyShadowTreeTransaction()|.  I would prefer to
stick this method on ShadowLayerManager if it's not too inconvenient.

>diff --git a/gfx/layers/ipc/CompositorParent.cpp b/gfx/layers/ipc/CompositorParent.cpp

> void
>+CompositorParent::ContentUpdated()

Similarly, |NotifyShadowTreeTransaction()|.

>@@ -861,16 +868,17 @@ CompositorParent::ShadowLayersUpdated(Sh

>   ScheduleComposition();
>+  mLayerManager->NotifyContentUpdated();

Should this just call NotifyShadowTreeTransaction().

>diff --git a/gfx/layers/opengl/LayerManagerOGL.cpp b/gfx/layers/opengl/LayerManagerOGL.cpp

>+void
>+LayerManagerOGL::FPSState::ContentUpdated()

And here.

r=me with that.  Will file followup for ... interesting ... fps
calculation.
Comment 3 Matt Woodrow (:mattwoodrow) 2012-08-06 19:52:43 PDT
Created attachment 649525 [details] [diff] [review]
Meter layers transactions in addition to composites

Fixed review comments, carrying forward r=cjones
Comment 4 Matt Woodrow (:mattwoodrow) 2012-08-06 20:02:50 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fb78d71f361
Comment 5 Matt Brubeck (:mbrubeck) 2012-08-06 22:01:12 PDT
Sorry, I had to back this out because it caused all native Android tests runs to crash/timeout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a525ac9349eb
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-25 19:06:52 PDT
I noticed that in

 void
+CompositorParent::NotifyShadowTreeTransaction()
+{
+  ShadowLayerManager *shadow = mLayerManager->AsShadowManager();
+  if (shadow) {
+    shadow->NotifyShadowTreeTransaction();
+  }
+  ScheduleComposition();
+}

we don't check if mLayerManager is null, which Composite() does.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-25 19:14:19 PDT
https://tbpl.mozilla.org/?tree=Try&rev=32af6c0fdfc8
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-25 19:15:14 PDT
Created attachment 655388 [details] [diff] [review]
Rebased, with added null check
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-25 19:56:21 PDT
Andreas, if the try run in comment 7 comes back green while I'm in the air, this would be great to land before the work week to help with perf analysis.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-25 21:17:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc34849f109
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-08-26 13:05:54 PDT
https://hg.mozilla.org/mozilla-central/rev/dbc34849f109

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