Expose Overfill Numbers to Javascript For Perf Automation

RESOLVED FIXED in 2.0 S1 (9may)

Status

()

defect
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

({perf})

unspecified
2.0 S1 (9may)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=automation p=4 s=2014.05.09 u=])

Attachments

(1 attachment, 8 obsolete attachments)

The best way to get overfill numbers into travis is to expose it to a JS call. Do that here.
This is calculated in the compositor and is not trivially available the JS. If a racy read-back is ok, you could make the value volatile and read it from the parent main thread during a JS call and then forward it to the child process if you need it there.
Yeah it should be fairly stable, so it doesn't have to be for the exact frame we're compositing. I think as long as it's readable and we're not layerizing in extreme ways while scrolling, the number should be stable.
I'm taking the basic approach of window.requestAnimationFrame. Introduce a window.requestOverfill(aCallback). aCallback will be called after the next Composite with the overfill numbers.
Comment on attachment 8388983 [details] [diff] [review]
Messy WIP - IPDL + XPCOM hookup

This should definitely be [ChromeOnly]
Posted patch timestamp.patch (obsolete) — Splinter Review
Messy WIP - Works on OS X & OMTC. Doesn't work on fxos. Will add [chrome-only] later. 

For now, the child compositor can't communicate with the parent compositor on b2g. I also see errno 10, meaning no child process from the parent when my test app spawns. Looking to see if there are ways to funnel the information without stuffing it into a layer transaction or adding another ipdl.
Attachment #8388983 - Attachment is obsolete: true
Posted patch overfill.patch (obsolete) — Splinter Review
Attachment #8390936 - Attachment is obsolete: true
I got one version working that stuffs the request and response in a LayerTransaction, but this kind of information isn't really a LayerTransaction. I looked into having some kind of CompositorQuery.ipdl, but then stumbled upon the CrossProcessParentCompositor, which looks like what I want. Still investigating through the CrossProcessParentCompositor.
Posted patch overfill.patch (obsolete) — Splinter Review
A functional patch that exposes the compositor overfill numbers through window.requestOverfill(callback). The callback executes after the next frame is composited and we have the overfill numbers. Next step is to make it [ChromeOnly]. It's still a very messy WIP. 

Since the CompositorParent needs a LayerTransactionParent and there's no direct link between CompositorParent / CrossProcessCompositorParent without a LayerTree, I put the request for overfill numbers in a LayerTransaction. At the next composite, we send the overfill numbers back through the CompositorChild on the content side, and finally up through the ClientLayerManager, which executes the callback. Seems like I cut through a lot of different layers in a not very clean way, but couldn't find a cleaner option. Adding another IPDL interface feels worse.
Attachment #8390937 - Attachment is obsolete: true
Posted patch overfill.patch (obsolete) — Splinter Review
Have a few minor diffs. Because this is only useful for bug 968290, and bug 968290 is blocked on marionette fixes that enables us to actually scroll the window, no point in landing this until those fixes are done. Will check back again next sprint.
Attachment #8392562 - Attachment is obsolete: true
Flags: needinfo?(mchang)
Flags: needinfo?(mchang)
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
Hey BenWa, just wanted to get your feedback on this system design. It feels super hacky but I couldn't find a better way as I rummaged through the code.

The current approach is to add an interface like window.requestOverfill(callback) in JS. The callback with a parameter containing the current overfill numbers will execute after the next frame is composited. We do this by getting the ClientLayerManager, request the overfill numbers through the LayerTransactionChild. Seems also like an odd system design that is a LayerTransactionChild. The LayerTransactionParent then fetches the CompositorParent and sets a flag to get overfill numbers. On the next Composite, we send the overfill numbers back through the CompositorChild, which then feeds up through the ClientLayerManager, which finally executes the callback. It seems like a very round about / ugly way to do it.

The part I dislike the most is that we have to go through a LayerTransaction to request the overfill, but send the data back through the CompositorChild. Ideally, we'd like to go through the CompositorChild / CompositorParent interface, but this didn't seem possible. The only link between the CrossProcessCompositorParent and CompositorParent was through a LayerTransactionParent->Id, and I'm not sure I should break that interface instead for something barely used. Another option was to add a new IPDL interface, but that also feels like overkill / worse when all we want is this window.requestOverfill interface for testing purposes. 

Any thoughts on a different /cleaner way to do this. Thanks!
Flags: needinfo?(bgirard)
Comment on attachment 8393897 [details] [diff] [review]
overfill.patch

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

This looks like a reasonable way to get the data.

Like I said I think overfill numbers are misleading and that it's a distracting from better (but harder) testing metric. As well if we're going to measure overfill we should expect these numbers of change very frequently and I think it's actually a very bad idea to put the burden of the developer to diagnose why the overfill numbers are regressing. That being said I wont block the collection of this if we just want to use it for general data.

::: content/base/public/nsIDocument.h
@@ +1842,4 @@
>    typedef mozilla::dom::CallbackObjectHolder<
>      mozilla::dom::FrameRequestCallback,
>      nsIFrameRequestCallback> FrameRequestCallbackHolder;
> +

Let's not modify this.

::: dom/base/nsGlobalWindow.h
@@ +1229,4 @@
>    nsRect GetInnerScreenRect();
>  
>    void ScrollTo(const mozilla::CSSIntPoint& aScroll);
> +  

whitespace only change

::: dom/webidl/Window.webidl
@@ +183,4 @@
>    void scroll(long x, long y);
>    void scrollTo(long x, long y);
>    void scrollBy(long x, long y);
> +  [ChromeOnly, Throws] void requestOverfill(OverfillCallback callback);

you'll want someone else to review this. They will likely tell you to use mozRequestOverfill

::: gfx/layers/Layers.h
@@ +599,4 @@
>    virtual bool IsCompositingCheap() { return true; }
>  
>    bool IsInTransaction() const { return mInTransaction; }
> +  virtual bool RequestOverfill(mozilla::dom::OverfillCallback* aCallback); 

These need documentation.

@@ +599,5 @@
>    virtual bool IsCompositingCheap() { return true; }
>  
>    bool IsInTransaction() const { return mInTransaction; }
> +  virtual bool RequestOverfill(mozilla::dom::OverfillCallback* aCallback); 
> +  virtual void RunOverfillCallback(const uint32_t aOverfill);

Wouldn't it be better to return a float for the overfill? We display an int because of our limited font system for the Compositor FPS displays.

::: gfx/layers/client/ClientLayerManager.cpp
@@ +276,5 @@
> +bool
> +ClientLayerManager::RequestOverfill(mozilla::dom::OverfillCallback* aCallback)
> +{
> +  printf_stderr("ClientLayerManager::Requesting overfill\n");
> +  CompositorChild* child = GetRemoteRenderer();

Looks like this line isn't needed.

@@ +277,5 @@
> +ClientLayerManager::RequestOverfill(mozilla::dom::OverfillCallback* aCallback)
> +{
> +  printf_stderr("ClientLayerManager::Requesting overfill\n");
> +  CompositorChild* child = GetRemoteRenderer();
> +  mOverfillCallback = aCallback;

This should be a list.

@@ +283,5 @@
> +  if (HasShadowManager()) {
> +    printf_stderr("ClientLayerManager::SendRequestOverfill\n");
> +    // The compositor child has a layer manager, but is
> +    // never actually created with it.
> +    CompositorChild::Get()->SetLayerManager(this);

This feel really hacky and I can't tell why it's really needed. We should implement this without it.
Attachment #8393897 - Flags: feedback+
Flags: needinfo?(bgirard)
Posted patch overfill.patch (obsolete) — Splinter Review
Thanks for the feedback. Updated to include your improvements. After digging more into the code, realized we have a global CompositorChild. Now the CompositorChild has a list of ClientLayerManagers* to notify when it receives an overfill request. 

> Wouldn't it be better to return a float for the overfill? We display an int 
> because of our limited font system for the Compositor FPS displays.

I personally like it as a size_t because then it's easier to match the overfill numbers with the ones being displayed on screen.

Since Marionnette scroll events are broken, will wait for review once I have a test case working as more modifications may be needed then.
Attachment #8393897 - Attachment is obsolete: true
Blocks: 990833
Depends on: 997377
Posted patch overfill.patch (obsolete) — Splinter Review
I have been able to run a make test-perf test case with scrolling and have confirmed that the gecko side works, so asking for review.
Since the last review, the architecture has changed a bit. The essentially idea is to have something like this in JS:

window.MozRequestOverfill(aCallback);

The callback will get called on the next composite with the fill ratio as a parameter. Previously, from comment 10, we had to go through the LayerTransactions. By adding ChromeOnly to the Window.idl, which is actually what we want, we don't have to use the LayerTransaction since the CompositorChild is in the main b2g process! Thus, we can directly communicate to the Compositor with CompositorParent/CompositorChild ipdl.

The overall flow is nsGlobalWindow::MozRequestOverfill, which calls ClientLayerManager. The ClientLayerManager subscribes as an overfill ratio observer, fetches the CompositorChild and requests the fill ratio numbers from the CompositorParent. The CompositorParent sets a flag to send the data back to the child. When the next composite occurs, we send the fill ratio back to the CompositorChild. The CompositorChild notifies all LayerManagers, which then execute the callback in JS. Please let me know if you have any questions. Thanks!
Attachment #8397463 - Attachment is obsolete: true
Attachment #8416275 - Flags: review?(bgirard)
Comment on attachment 8416275 [details] [diff] [review]
overfill.patch

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

Looks great overall, just minor changes. I'll like to see the patch again.

::: content/base/public/nsIDocument.h
@@ +105,4 @@
>  class Event;
>  class EventTarget;
>  class FrameRequestCallback;
> +class OverfillCallback;

Can we remove this?

::: dom/base/nsGlobalWindow.cpp
@@ +7083,5 @@
> +    mozilla::layers::LayerManager* manager = widget->GetLayerManager();
> +    if (manager) {
> +      mozilla::dom::OverfillCallback* callback = &aCallback;
> +      manager->RequestOverfill(callback);
> +      printf_stderr("Finished Requesting Overfill\n");

No need to log success condition.

@@ +7086,5 @@
> +      manager->RequestOverfill(callback);
> +      printf_stderr("Finished Requesting Overfill\n");
> +    }
> +  } else {
> +    printf_stderr("[MozRequestOverfill] - Error: No Widget\n");

We should avoid printf_stderr in optimized builds. Normally we prefer NS_ASSERTION

::: dom/webidl/Window.webidl
@@ +203,5 @@
> + * and is an indicator that we're layerizing correctly.
> + * This function will call the given callback at the next composite, with the
> + * current fill ratio for the composited frame.
> + */
> +partial interface Window {

webidl and nsGlobalWindow will need a review for the appropriate peers. I'm not qualified to review this change.

::: gfx/layers/client/ClientLayerManager.cpp
@@ +289,5 @@
> +bool
> +ClientLayerManager::RequestOverfill(mozilla::dom::OverfillCallback* aCallback)
> +{
> +  printf_stderr("ClientLayerManager::Requesting overfill\n");
> +  MOZ_ASSERT(aCallback != NULL);

prefer nullptr over NULL.

@@ +295,5 @@
> +  if (HasShadowManager()) {
> +    printf_stderr("ClientLayerManager::SendRequestOverfill\n");
> +    CompositorChild* child = GetRemoteRenderer();
> +    if (!child) {
> +      printf_stderr("Could not get CompositorChild, returning\n");

MOZ_ASSERT. Let's not printf_stderr in production code.

@@ +303,5 @@
> +    child->AddOverfillObserver(this);
> +    child->SendRequestOverfill();
> +    mOverfillCallbacks.AppendElement(aCallback);
> +  } else {
> +    printf_stderr("[MozRequestOverfill] - Only supported on B2G Platform\n");

This needs to be removed but this feature should only depend on OMTC, not b2g.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +853,5 @@
> +bool
> +CompositorParent::RecvRequestOverfill()
> +{
> +  printf_stderr("CompositorParent::RecvRequestOverfill\n");
> +  mSendOverfillData = true;

Why not call mCompositor->GetFillRatio now? It should give you the number for the previous frame. Either way you're racing and unsure which frame you're going to get the data for.

This way you're also sure you're going to get a number back. If you wait for the next frame you might not have a next frame pending.

::: gfx/layers/ipc/PCompositor.ipdl
@@ +45,5 @@
>    // the root layer tree).
>    async DidComposite(uint64_t id);
>  
> +  // The parent sends the child the requested fill ratio numbers.
> +  async Overfill(uint32_t aOverfill);

async, yay!
Attachment #8416275 - Flags: review?(bgirard) → review-
Updated with fixes from comment 14. 

@Blake - The overall idea is that in chrome only code, we can do window.mozRequestOverfill(callback). The callback is executed once we pull a metric out from the compositor in the graphics side. Your review would be helpful in the DOM portions, specifically the window.idl and nsGlobalWindow implementation. Thanks!

> ::: content/base/public/nsIDocument.h
> @@ +105,4 @@
>  class Event;
>  class EventTarget;
>  class FrameRequestCallback;
> +class OverfillCallback;
>
> Can we remove this?

I couldn't find an alternate way. I think this is what happens with all the magic from the .idl compiler, so there's no place to include the actual header file where the class is defined. I was essentially following the mozRequestAnimationFrame implementation, which exists here too.
Attachment #8416275 - Attachment is obsolete: true
Attachment #8418255 - Flags: review?(mrbkap)
Attachment #8418255 - Flags: review?(bgirard)
Also, forgot to say, architecturally I updated the patch to just return the last fill ratio instead of setting a flag and returning the fill ratio at the next composite. Good catch!
Comment on attachment 8418255 [details] [diff] [review]
Expose Compositor Fill Ratio to Chrome JS

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

::: gfx/layers/client/ClientLayerManager.h
@@ +229,4 @@
>  
>    RefPtr<ShadowLayerForwarder> mForwarder;
>    nsAutoTArray<RefPtr<TextureClientPool>,2> mTexturePools;
> +  nsAutoTArray<dom::OverfillCallback*,4> mOverfillCallbacks;

You're preallocating room for 4 entries inline. In the general case this feature wont be used. Shouldn't that be 0 (should check if the default value is in-fact zero)? The run-time cost will be dominated by the IPC message so we wont be making the operation overall faster here anyways. I suspect we might be required to ship more code for this template instantiation too. I don't think this is worth it.

::: gfx/layers/ipc/CompositorChild.cpp
@@ +139,5 @@
>  }
>  
> +bool
> +CompositorChild::RecvOverfill(const uint32_t &aOverfill) {
> +  printf_stderr("CompositorChild::RecvOverfill() %d\n", aOverfill);

Remove debugging code.
Attachment #8418255 - Flags: review?(bgirard) → review+
Comment on attachment 8418255 [details] [diff] [review]
Expose Compositor Fill Ratio to Chrome JS

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

r=me with my questions/comments addressed.

::: dom/base/nsGlobalWindow.cpp
@@ +7075,5 @@
>    }
>  }
>  
> +void
> +nsGlobalWindow::MozRequestOverfill(mozilla::dom::OverfillCallback& aCallback, mozilla::ErrorResult& aError)

Nit: This file is already 'using mozilla' and 'using mozilla::dom' so you can make the definition a little shorter.

@@ +7082,5 @@
> +  if (widget) {
> +    mozilla::layers::LayerManager* manager = widget->GetLayerManager();
> +    if (manager) {
> +      mozilla::dom::OverfillCallback* callback = &aCallback;
> +      manager->RequestOverfill(callback);

I'd just pass &aCallback and skip the named variable.

::: dom/webidl/Window.webidl
@@ +204,5 @@
> + * This function will call the given callback current fill ratio for a
> + * composited frame. We don't guarantee which frame fill ratios will be returned.
> + */
> +partial interface Window {
> +  [ChromeOnly, Throws] void mozRequestOverfill(OverfillCallback callback);

Can this actually throw? Should we be throwing if we don't successfully set up the callback?

::: gfx/layers/ipc/CompositorChild.cpp
@@ +138,5 @@
>    return true;
>  }
>  
> +bool
> +CompositorChild::RecvOverfill(const uint32_t &aOverfill) {

Nit: This brace should be on its own line.
Attachment #8418255 - Flags: review?(mrbkap) → review+
(In reply to Benoit Girard (:BenWa) from comment #17)
> Comment on attachment 8418255 [details] [diff] [review]
> Expose Compositor Fill Ratio to Chrome JS
> 
> Review of attachment 8418255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/ClientLayerManager.h
> @@ +229,4 @@
> >  
> >    RefPtr<ShadowLayerForwarder> mForwarder;
> >    nsAutoTArray<RefPtr<TextureClientPool>,2> mTexturePools;
> > +  nsAutoTArray<dom::OverfillCallback*,4> mOverfillCallbacks;
> 
> You're preallocating room for 4 entries inline. In the general case this
> feature wont be used. Shouldn't that be 0 (should check if the default value
> is in-fact zero)? The run-time cost will be dominated by the IPC message so
> we wont be making the operation overall faster here anyways. I suspect we
> might be required to ship more code for this template instantiation too. I
> don't think this is worth it.

The default is 0, updated to use 0.
Attachment #8418255 - Attachment is obsolete: true
Attachment #8419824 - Flags: review+
Mostly successful try - https://tbpl.mozilla.org/?tree=Try&rev=7a658aced906 - The try servers seemed to die halfway and Windows 7 / 8 don't seem to be running on anyone elses try pushes as well. Since this is an optional code path that shouldn't be used by anyone, adding checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b3213ee2a85d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [c=automation p=4 s= u=] → [c=automation p=4 s=2014.05.09 u=]
Target Milestone: 1.4 S5 (11apr) → 2.0 S1 (9may)
Blocks: 1006567
Blocks: 1006129
No longer blocks: 1006567
You need to log in before you can comment on or make changes to this bug.