Closed Bug 1077651 Opened 5 years ago Closed 4 years ago

Create a benchmark to measure compositor alignment to vsync and detect regressions

Categories

(Core :: Graphics, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
tracking-b2g backlog
Tracking Status
firefox41 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

(Keywords: perf, Whiteboard: [perf-wanted])

Attachments

(20 files, 15 obsolete files)

3.36 MB, text/plain
Details
3.44 MB, text/plain
Details
1.37 KB, application/zip
Details
135.88 KB, image/png
Details
131.89 KB, image/png
Details
105.27 KB, image/png
Details
5.15 KB, text/plain
Details
5.14 KB, text/plain
Details
4.14 KB, text/plain
Details
5.13 KB, text/plain
Details
5.21 KB, text/plain
Details
5.13 KB, text/plain
Details
94 bytes, text/plain
Details
94 bytes, text/plain
Details
1.87 MB, application/pdf
Details
29.35 KB, application/pdf
Details
26.29 KB, application/pdf
Details
55 bytes, text/plain
Details
523.16 KB, application/pdf
Details
35.75 KB, patch
mrbkap
: review+
mchang
: review+
Details | Diff | Splinter Review
Come up with a test case for Project Silk where we can show that the compositor performance is better aligned to vsync versus not aligned to vsync. Measure this and make this a default benchmark.
Attached file Progress Bar Animation
A Progress bar animation that I've been using to test OMTA. What we see on master versus silk is quite interesting. Silk aligns perfectly to vsync and composites within ~3-4 ms most of the time. Master aggregates and starts to align to vsync in the beginning, trails behind, then composites take longer as the driver starts blocking to grab mutexes (1076166)
Desktop version of the app - http://codepen.io/achudars/pen/bgcsp
Attached image OMTa Screenshot Silk
Attached image Screenshot Of Bug
Here's an interesting design point - If a layer transaction comes at around the same time as a vsync event, we will skip a frame as the vsync might occur before the schedule composite.
Measured between CompositorParent::CompositeToTarget. We print out a histogram of how many milliseconds each composite took for 500 frames and calculate the average / std deviation.
We see a nice reduction in overall composite times as well since we're no longer blocked by the driver locking on vsync.
Summary: Compositor Performance Test for OMTA → Compositor Performance Tests and Results for Silk
Very nice reduction in composite times. I just scrolled back/forth on the homescreen.
Attached patch Patch to Measure Composite Times (obsolete) — Splinter Review
In case anyone needs to reproduce.
Does the 20th bucket capture >= 20? And why aren't we using telemetry for this and actually report this back to us from the field?
Very nice results while performing the app launch animation.
Some thoughts on compositor performance metrics that we can use as a way to compare silk versus master:

1) Composite times on OMTA animations / Scrolling - So far this seems pretty good. Since the HwC driver isn't blocking anymore, this should help with frame throughput as well for things like layer transactions.
2) Missed composites - We can measure how many frames did not complete before a vsync. This is really the ultimate measurement that the frame was good, but we can't really measure this on master, which may lead us to just use (1) to measure before/after, and use (2) as a final QA / we didn't regress measurement.

What do you guys think?
Flags: needinfo?(hshih)
Flags: needinfo?(cku)
(In reply to Andreas Gal :gal from comment #13)
> Does the 20th bucket capture >= 20? And why aren't we using telemetry for
> this and actually report this back to us from the field?

Yes the 20th bucket captures >= 20. I should look into hooking this into telemetry. For now it was just a quick way to measure composite times and how we're doing.
See Also: → 1080160
(In reply to Mason Chang [:mchang] from comment #18)
> Some thoughts on compositor performance metrics that we can use as a way to
> compare silk versus master:
> 
> 1) Composite times on OMTA animations / Scrolling - So far this seems pretty
> good. Since the HwC driver isn't blocking anymore, this should help with
> frame throughput as well for things like layer transactions.
> 2) Missed composites - We can measure how many frames did not complete
> before a vsync. This is really the ultimate measurement that the frame was

How can we know the frame we don't complete? Is this used for measuring the refresh driver performance?

> good, but we can't really measure this on master, which may lead us to just
> use (1) to measure before/after, and use (2) as a final QA / we didn't
> regress measurement.
> 
> What do you guys think?

For omta case, we can show the layer position.
Boris's patch now can dump the color layer's position and dump a easy-read graph, and he will upload the
silk and master result soon.
Flags: needinfo?(hshih)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #20)
> (In reply to Mason Chang [:mchang] from comment #18)
> > Some thoughts on compositor performance metrics that we can use as a way to
> > compare silk versus master:
> > 
> > 1) Composite times on OMTA animations / Scrolling - So far this seems pretty
> > good. Since the HwC driver isn't blocking anymore, this should help with
> > frame throughput as well for things like layer transactions.
> > 2) Missed composites - We can measure how many frames did not complete
> > before a vsync. This is really the ultimate measurement that the frame was
> 
> How can we know the frame we don't complete? Is this used for measuring the
> refresh driver performance?

If we composite at a vsync event, we should finish compositing before the the next vsync (e.g. before 16.6 ms). This doesn't require the refresh driver necessarily. Another way to think about it is, did we finish compositing within the frame's 16.6 ms budget. Does that make sense?
(In reply to Boris Chiou [:boris] from comment #24)
> Created attachment 8504567 [details]
> The statistics of movement data on OMTA app
> 
> The statistics of movement data on OMTA app.
> The calculation is finished by CJ's python tool.
> (https://github.com/CJKu/pysilk)
> According to the statistics, the standard deviations and CVs with Silk are
> lower than those without Silk. They look good to me.
> 
> 1. master
>   a. Test sample 01:
>      * Mean value         = 4.972
>      * Standard deviation = 0.494
>      * CV                 = 9.939%
>   b. Test sample 02:
>      * Mean value         = 4.969
>      * Standard deviation = 0.445
>      * CV                 = 8.956%
>   c. Test sample 03:
>      * Mean value         = 4.971
>      * Standard deviation = 0.496
>      * CV                 = 9.981%
> 2. silk-compositor-only
>   a. Test sample 01:
>      * Mean value         = 4.993
>      * Standard deviation = 0.366
>      * CV                 = 7.330%
>   b. Test sample 02:
>      * Mean value         = 4.983
>      * Standard deviation = 0.355
>      * CV                 = 7.115%
>   c. Test sample 03:
>      * Mean value         = 4.996
>      * Standard deviation = 0.350
>      * CV                 = 7.005%

When looking at the details statistics, I see that we have some things like this:

01.
Frame number       = 10 ~ 2016

02.
Frame number       = 3329 ~ 5336

What happened to frames 2017 - 3328? Why did we discard those ranges?
Flags: needinfo?(boris.chiou)
(In reply to Mason Chang [:mchang] from comment #27)
> (In reply to Boris Chiou [:boris] from comment #24)
> > Created attachment 8504567 [details]
> > The statistics of movement data on OMTA app
> > 
> > The statistics of movement data on OMTA app.
> > The calculation is finished by CJ's python tool.
> > (https://github.com/CJKu/pysilk)
> > According to the statistics, the standard deviations and CVs with Silk are
> > lower than those without Silk. They look good to me.
> > 
> > 1. master
> >   a. Test sample 01:
> >      * Mean value         = 4.972
> >      * Standard deviation = 0.494
> >      * CV                 = 9.939%
> >   b. Test sample 02:
> >      * Mean value         = 4.969
> >      * Standard deviation = 0.445
> >      * CV                 = 8.956%
> >   c. Test sample 03:
> >      * Mean value         = 4.971
> >      * Standard deviation = 0.496
> >      * CV                 = 9.981%
> > 2. silk-compositor-only
> >   a. Test sample 01:
> >      * Mean value         = 4.993
> >      * Standard deviation = 0.366
> >      * CV                 = 7.330%
> >   b. Test sample 02:
> >      * Mean value         = 4.983
> >      * Standard deviation = 0.355
> >      * CV                 = 7.115%
> >   c. Test sample 03:
> >      * Mean value         = 4.996
> >      * Standard deviation = 0.350
> >      * CV                 = 7.005%
> 
> When looking at the details statistics, I see that we have some things like
> this:
> 
> 01.
> Frame number       = 10 ~ 2016
> 
> 02.
> Frame number       = 3329 ~ 5336
> 
> What happened to frames 2017 - 3328? Why did we discard those ranges?

We capture the data for several times. The frame number is still increased between each capture.
So we will see some frame missed. In one captured data, the frame number should continuously.
Flags: needinfo?(boris.chiou)
Attachment #8504562 - Attachment is obsolete: true
Attachment #8504564 - Attachment is obsolete: true
Attachment #8504567 - Attachment is obsolete: true
Attachment #8504655 - Attachment is obsolete: true
Attached file Compositor Noise Measurements (obsolete) —
While running our test OMTA app - https://github.com/changm/OMTA-Examples - Bounce the fox, I measured master vs HwcComposer vsync's intervals between composites. Master is using the software timer and HwcComposer is using the timestamp provided by the hardware. All measurements are in microseconds. Each run here represents 3600 captured frames, or roughly 1 minute worth of animation. We see significant reduction in noise and the hardware composer is significantly better. Each interval on average is 16.6783 ms with a pretty tight interval. In the worst case, we have a 55 microsecond variance, compared to over 1ms worth of variance with the software timer.
Attachment #8516241 - Attachment is obsolete: true
Attached file OMTA Layer Position
The layer position for OMTA animations are based on the timestamp here - http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?from=TransformShadowTree&case=true#985. Here are measurements based on layer transform uniformity, using the same test procedures as Comment 30. We measure the shadow-transform set during the OMTA over 3600 frames for each run. Thus we have a total of 10 minutes of data. Lower standard deviation and closer clustering to the average implies a smoother animation. We see that's the case here. Silk has a tigher clustering to the average, with 8 / 10 runs being 0.05 pixels from the average and a more uniform std deviation.
Here is a 240 FPS video of an OMTA animation with and without silk. The device on the left has silk enabled. I personally can't see a difference, but maybe someone with a better eye for these things can.
(In reply to Mason Chang [:mchang] from comment #33)
> Created attachment 8516778 [details]
> 240 FPS Video of OMTA Animation
> 
> Here is a 240 FPS video of an OMTA animation with and without silk. The
> device on the left has silk enabled. I personally can't see a difference,
> but maybe someone with a better eye for these things can.

I looked at the movie without reading the description, and hoped the left one was with silk on, because I thought it was smoother.  Or, more to the point, I thought the one on the right had a few rough spots, where the one on the left stayed smooth.
feature-b2g: --- → 2.2+
I pulled the newest code yesterday and re-did the layer movement testing. After turning on the preference of vsync..hw-vsync and vsync..compositor, I see the movement is much stable with vsync. Maybe it is due to our new timestamp change because master branch uses the current time, but vsync uses the timestamp passed from hw.
Flags: needinfo?(cku)
Changing summary to reflect the goal in comment 0 more clearly.
Blocks: 1078160
Summary: Compositor Performance Tests and Results for Silk → Create a benchmark to measure compositor alignment to vsync and detect regressions
remove from 2.2 spotlight.
feature-b2g: 2.2+ → ---
We can't ship Silk without this, so why removing 2.2+?
feature-b2g: --- → 2.2+
Milan, 2.2 branch will be created soon. Are we still targeting to land this before the branch is created? Thanks.
We won't land this before the branch is created, but will uplift instead.
Removing from feature-b2g nom as I discussed offline with mason and determined that this isn't a blocker for 2.2
feature-b2g: 2.2+ → ---
WIP - Uses APZ and native synthesized events to measure frame uniformity. It does this by setting a pref to true and hooking into requestAnimationFrame and dispatching a touch event at each frame. On the compositor side, it captures the transform applied at each composite on layers with an APZ on them while the pref is true. At the end of the test, the pref is shut off and calculate the frame uniformity for the time while the pref was enabled. We hook into the JS window object to request the frame uniformity data from the compositor.
Part 1: Adds a mochitest that requires APZ to run. It sends a constant scroll event every requestAnimationFrame and at the end and checks that the frame uniformity is ok. The pref is a live preference that tells the compositor to collect layer transforms while the preference is true. This is a chrome mochitest since we add a new MozGetFrameUniformity() on the window object that is chrome only.
Attachment #8603108 - Attachment is obsolete: true
Attachment #8603447 - Flags: review?(bugmail.mozilla)
Part 2: Modifies the AsyncCompositionManager where we sample APZ animations to record APZ transforms. Even though we don't actually have any APZ animations since we're sending touch events, it's a good place to hook into APZ to see the transforms applied by the touch events.
Attachment #8502011 - Attachment is obsolete: true
Attachment #8603448 - Flags: review?(bugmail.mozilla)
Adds a new file called ScrollData. It records layer transforms applied during composites. When EndTest() is called, it calculates the frame uniformity of a layer. Right now it only supports 1 layer. I couldn't find a clean way to find only 1 layer that was moving since lots of layers during composites still seem to be in a panning state according to APZ. The Frame Uniformity is pretty much the standard deviation of movements between each composite.
Attachment #8603450 - Flags: review?(bugmail.mozilla)
Add the actual MozGetFrameUniformity as a chrome only function to the window object. It digs through the client layer manager, sends a sync IPC call to the Compositor, which gets the frame uniformity data from the AsyncCompositionManager. I'm ok with a sync call here since it should only be used for testing purposes and adding callback support is lots of extra code.
Attachment #8603453 - Flags: review?(bugmail.mozilla)
Comment on attachment 8603447 [details] [diff] [review]
Part 1: Add mochitest to test frame uniformity. r=kats

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

Please redo these patches so that they are split by function rather than file. Patches shouldn't depend on future parts; it's more useful to just lump everything into a single patch since at least that way I don't have to keep going back and forth between different patches.

::: gfx/layers/apz/test/chrome.ini
@@ +2,5 @@
> +support-files =
> +  apz_test_utils.js
> +  apz_test_native_event_utils.js
> +  helper_bug982141.html
> +  helper_bug1151663.html

You don't need any of these helpers except native_event_utils

::: gfx/layers/apz/test/test_smoothness.html
@@ +17,5 @@
> +  <script type="text/javascript">
> +    var scrollEvents = 100;
> +    var i = 0;
> +    var scrollDiv = document.getElementById("scrollbox");
> +    var testPref = "gfx.vsync.collect-scroll-data"

add a ;

@@ +36,5 @@
> +        SpecialPowers.setBoolPref(testPref, false);
> +        var frameUniformity = window.mozGetFrameUniformity();
> +        // Locally, with silk and apz + e10s, retina 15" mbp usually get ~1.0 - 1.5
> +        // w/o silk + e10s + apz, I get up to 7. Lower is better.
> +        SimpleTest.ok(frameUniformity < 2.0, "Frame uniformity is too low.");

What values do you get on other platforms? Windows/linux? Are the numbers the same on tryserver?

@@ +50,5 @@
> +        SimpleTest.finish();
> +      }
> +
> +      SimpleTest.waitForExplicitFinish();
> +      SpecialPowers.setBoolPref(testPref, true);

waitForExplicitFinish should run outside the onload, I think. Also you probably want to use pushPrefEnv instead of setBoolPref
Attachment #8603447 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8603450 [details] [diff] [review]
Part3: Add ScrollData to record scroll transforms

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

::: gfx/layers/composite/ScrollData.cpp
@@ +7,5 @@
> +#include "mozilla/TimeStamp.h"
> +#include "gfxPoint.h"
> +#include "nsTArray.h"
> +#include <map>
> +#include "ScrollData.h"

see section on #include ordering at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

@@ +22,5 @@
> +
> +void
> +ScrollData::AddMeasurement(TimeStamp aTimestamp, const void* aLayerAddress, Point aTransform)
> +{
> +  LayerTransforms* LayerTransforms = GetLayerTransforms(aLayerAddress);

camelcase variable name, here and throughout

@@ +56,5 @@
> +{
> +  MOZ_ASSERT(!aLayerTransforms->mTransforms.IsEmpty());
> +
> +  Point current = aLayerTransforms->mTransforms[0];
> +  Point average(0.0, 0.0);

It gets initialized to 0 by default, here and throughout

@@ +57,5 @@
> +  MOZ_ASSERT(!aLayerTransforms->mTransforms.IsEmpty());
> +
> +  Point current = aLayerTransforms->mTransforms[0];
> +  Point average(0.0, 0.0);
> +  double length = aLayerTransforms->mTransforms.Length();

why double? Shouldn't this be a size_t? You can cast it to float for the division later.

@@ +67,5 @@
> +    current = nextTransform;
> +  }
> +
> +  average.x /= length;
> +  average.y /= length;

average = average / length;

@@ +132,5 @@
> +
> +    for (size_t i = 0; i < LayerTransforms->mTransforms.Length(); i++) {
> +      Point transform = LayerTransforms->mTransforms[i];
> +      // Only scrolls with enough data
> +      if ((transform != origin) && (LayerTransforms->mTransforms.Length() > 10)) {

what if there are 11 samples but 10 of them are at the origin and only the last one is nonzero?

@@ +160,5 @@
> +    LayerTransforms* LayerTransforms = iter->second;
> +    Point stdDev = GetStdDev(LayerTransforms);
> +    yUniformity = stdDev.y;
> +  }
> +  MOZ_ASSERT(mFrameTransforms.size() == 1);

This should be at the top

::: gfx/layers/composite/ScrollData.h
@@ +20,5 @@
> +class ScrollData {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ScrollData);
> +
> +public:
> +  explicit ScrollData() {}

No need for |explicit| here

@@ +31,5 @@
> +  ~ScrollData();
> +
> +private:
> +  gfx::Point GetAverage(LayerTransforms* aLayerTransforms);
> +  gfx::Point GetStdDev(LayerTransforms* aLayerTransforms);

These two function should be public functions on the LayerTransforms struct (better OOP practice)

@@ +41,5 @@
> +
> +}
> +}
> +
> +#endif // mozilla_layers_opengl_FPSCounter_h_

copypasta error
Attachment #8603450 - Flags: review?(bugmail.mozilla)
Comment on attachment 8603453 [details] [diff] [review]
Part 4: Add MozGetFrameUniformity to window.

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

This would be better on DOMWindowUtils. There's no need to expose it on the window.
Attachment #8603453 - Flags: review?(bugmail.mozilla) → review-
Updated with feedback from comment 47-49. Updated to use DOMWindowUtils. Also, instead of just requesting frame uniformity, I changed ScrollData to track based on the APZ ScrollableLayerGuid. When we request frame uniformity, the touch point is sent and we use hit testing from APZ to find the correct ScrollableLayerGuid to find the uniformity value. Also the test is marked valid only on Windows / Mac since they have hardware vsync. Will test on try to see their values.
Attachment #8603447 - Attachment is obsolete: true
Attachment #8603448 - Attachment is obsolete: true
Attachment #8603450 - Attachment is obsolete: true
Attachment #8603453 - Attachment is obsolete: true
Attachment #8605405 - Flags: review?(bugmail.mozilla)
Comment on attachment 8605405 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

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

I have to go to TRIBE so I didn't finish reviewing fully, but some pieces need fixing. In particular the API needs to define what it returns for error conditions and I think you want to be using the shadow transform rather than the transform. See comments below.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1809,5 @@
>    attribute boolean serviceWorkersTestingEnabled;
> +
> +  /**
> +   * Calculates the frame uniformity of the Y axis on a scroll
> +   * in conjunction with the gfx pref gfx.vsync.collect-scroll-data

"in conjunction with" isn't a great description of the relationship. Also this comment doesn't say anything about what the parameters mean. I'd prefer something like:

"Returns the frame uniformity of the Y axis on a scroll which is computed when the pref gfx.vsync.collect-scroll-data is enabled. The x,y coordinates passed are used to identify the layer whose frame uniformity are returned"

You should also specify that a return of -1 (or maybe all negative values) indicate no frame uniformity data was available.

@@ +1811,5 @@
> +  /**
> +   * Calculates the frame uniformity of the Y axis on a scroll
> +   * in conjunction with the gfx pref gfx.vsync.collect-scroll-data
> +   */
> +   float GetFrameUniformity(in int32_t aX, in int32_t aY);

Rename to start with a lowercase 'g'

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +1258,5 @@
> +APZCTreeManager::GetScrollableLayerGuid(const ScreenPoint& aPoint, ScrollableLayerGuid& aGuid)
> +{
> +  HitTestResult hitTestResult;
> +  nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(aPoint, &hitTestResult);
> +  if (apzc && (hitTestResult != HitTestResult::HitNothing)) {

I don't think you need to check or even bother getting the HitTestResult here. If apzc is non-null the hitTestResult must be != HitNothing. You can just pass in nullptr for the second argument of GetTargetAPZC, similar to how HitTestAPZC does it.

@@ +1259,5 @@
> +{
> +  HitTestResult hitTestResult;
> +  nsRefPtr<AsyncPanZoomController> apzc = GetTargetAPZC(aPoint, &hitTestResult);
> +  if (apzc && (hitTestResult != HitTestResult::HitNothing)) {
> +    aGuid =  apzc->GetGuid();

nit: extra ws after =

::: gfx/layers/apz/test/test_smoothness.html
@@ +30,5 @@
> +        i++;
> +        // Scroll diff
> +        var dx = 0;
> +        var dy = -10; // Negative to scroll down
> +        synthesizeNativeWheelAndWaitForEvent(scrollDiv, x, y, dx, dy, undefined);

Don't need to pass undefined explicitly, you can just omit that argument.

@@ +34,5 @@
> +        synthesizeNativeWheelAndWaitForEvent(scrollDiv, x, y, dx, dy, undefined);
> +        window.requestAnimationFrame(sendScrollEvent);
> +      } else {
> +        SpecialPowers.popPrefEnv(SimpleTest.finish);
> +        var frameUniformity = _getDOMWindowUtils().GetFrameUniformity(x, y);

s/GetFrame/getFrame/

@@ +37,5 @@
> +        SpecialPowers.popPrefEnv(SimpleTest.finish);
> +        var frameUniformity = _getDOMWindowUtils().GetFrameUniformity(x, y);
> +        // Locally, with silk and apz + e10s, retina 15" mbp usually get ~1.0 - 1.5
> +        // w/o silk + e10s + apz, I get up to 7. Lower is better.
> +        // Windows, I get ~3. Values are not valid w/o hardware vsync

So.. negative values are ok? How do you detect valid cases vs invalid cases where you return -1 because there's no data available? The API needs to be stricter

@@ +50,5 @@
> +
> +    window.onload = function() {
> +      var apzEnabled = SpecialPowers.getBoolPref("layers.async-pan-zoom.enabled");
> +      if (!apzEnabled) {
> +        SimpleTest.ok(true, "APZ not enabled, skipping test");

Is there a mochitest test condition like skip-if(!asyncPanZoomEnabled) ? If so I'd rather use that so we skip this check entirely. That will also be safer because otherwise we'll need to adjust this to deal with the changes in bug 1162064.

::: gfx/layers/client/ClientLayerManager.cpp
@@ +428,5 @@
>  
> +float
> +ClientLayerManager::GetFrameUniformity(const ScreenIntPoint& aTouchPoint)
> +{
> +  MOZ_ASSERT(XRE_IsParentProcess(), "Frame Uniformity only supported in parent process");

I suppose this makes it simpler but conceptually there's no reason we can't do it from the child process too. This fine for now, I guess we can add the child process code if we need it.

@@ +431,5 @@
> +{
> +  MOZ_ASSERT(XRE_IsParentProcess(), "Frame Uniformity only supported in parent process");
> +  if (HasShadowManager()) {
> +    CompositorChild* child = GetRemoteRenderer();
> +    float frameUniformity = 0.0;

Initialize to -1?

@@ +433,5 @@
> +  if (HasShadowManager()) {
> +    CompositorChild* child = GetRemoteRenderer();
> +    float frameUniformity = 0.0;
> +    IntPoint touchPoint(aTouchPoint.x, aTouchPoint.y);
> +    child->SendGetFrameUniformity(touchPoint, &frameUniformity);

Keep this as a ScreenIntPoint rather than turning it into an IntPoint and the back on the compositor side. You may need to add a |using mozilla::ScreenIntPoint from "mozilla/Units.h"| to PCompositor.ipdl or something to that effect.

@@ +437,5 @@
> +    child->SendGetFrameUniformity(touchPoint, &frameUniformity);
> +    return frameUniformity;
> +  }
> +
> +  return -1;

Would prefer |return LayerManager::GetFrameUniformity| (i.e. delegate to the base class default return value rather than hard-coding it twice).

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +99,5 @@
> +    : mLayerManager(aManager)
> +    , mIsFirstPaint(true)
> +    , mLayersUpdated(false)
> +    , mReadyForCompose(true)
> +    , mScrollMeasurements(new ScrollData())

Reduce indent of this stuff by two spaces

@@ +560,5 @@
> +    if (gfxPrefs::CollectScrollData()) {
> +      gfx::Matrix4x4 layerTransform = aLayer.GetTransform();
> +      if (layerTransform.Is2DIntegerTranslation() && !layerTransform.IsIdentity()) {
> +        ScrollableLayerGuid scrollGuid = apzc->GetGuid();
> +        Point transform = layerTransform.As2D().GetTranslation();

Doesn't seem correct to be using aLayer.GetTransform() since that's just going to return the content-side translation, not the compositor-side async position. I think you want to be using the shadow transform? And even that, you don't want to be doing it in SampleAPZAnimations because this runs before the shadow transform is actually computed and applied. I think the cleanest might be to add another "pass" of the layer tree at the bottom of AsyncCompositionManager::TransformShadowTree and just walk the entire tree and store the shadow transform translation values.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1321,5 @@
> +CompositorParent::RecvGetFrameUniformity(const IntPoint& aTouchPoint, float* aFrameUniformity)
> +{
> +  ScreenIntPoint touchPoint(aTouchPoint.x, aTouchPoint.y);
> +  ScrollableLayerGuid scrollLayer;
> +  mApzcTreeManager->GetScrollableLayerGuid(touchPoint, scrollLayer);

You need to check the return value of this. Or not bother having a return value.

@@ +1714,5 @@
>    }
>  
> +  virtual bool RecvGetFrameUniformity(const IntPoint& aTouchPoint, float* aFrameUniformity)
> +  {
> +    *aFrameUniformity = -1.0;

Add a comment here that we don't support getting the frame uniformity from a child process so this is just a stub.

::: gfx/layers/ipc/CompositorParent.h
@@ +228,5 @@
>    CloneToplevel(const InfallibleTArray<mozilla::ipc::ProtocolFdMapping>& aFds,
>                  base::ProcessHandle aPeerProcess,
>                  mozilla::ipc::ProtocolCloneContext* aCtx) override;
>  
> +  virtual bool RecvGetFrameUniformity(const IntPoint& aTouchPoint, float* aFrameUniformity) override;

ScreenIntPoint

::: gfx/layers/ipc/PCompositor.ipdl
@@ +74,5 @@
>    // Child sends the parent a request for fill ratio numbers.
>    async RequestOverfill();
>  
> +  // Child requests frame uniformity measurements
> +  sync GetFrameUniformity(IntPoint aTouchPoint)

ScreenIntPoint

::: gfx/thebes/gfxPrefs.h
@@ +237,1 @@
>  

Move the blank line to above this pref, so this new pref is grouped with gfx.vsync.*
Attachment #8605405 - Flags: review?(bugmail.mozilla) → review-
Updated with feedback from comment 51. 

> Is there a mochitest test condition like skip-if(!asyncPanZoomEnabled) ? If 
> so I'd rather use that so we skip this check entirely. That will also be 
> safer because otherwise we'll need to adjust this to deal with the changes in > bug 1162064.

From the patches in bug 1162064, it looks like a preference wouldn't work anyway? APZ might be both e10s/widget dependent, which means we should add a check on the actual widget first?
Attachment #8605405 - Attachment is obsolete: true
Attachment #8605957 - Flags: review?(bugmail.mozilla)
Comment on attachment 8605957 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

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

Got a bunch of comments. Nothing major but I want to apply and run this locally before giving it an r+ because I want to see it in action.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1814,5 @@
> +   * The x,y coordinates passed are used to identify the layer
> +   * whose frame uniformity is returned. A negative value indicates
> +   * an invalid frame uniformity and an error has occured.
> +   */
> +   float getFrameUniformity(in int32_t aX, in int32_t aY);

Un-indent by one space

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +395,5 @@
>    already_AddRefed<AsyncPanZoomController> GetTargetAPZC(const ScreenPoint& aPoint,
>                                                           HitTestResult* aOutHitResult);
>    gfx::Matrix4x4 GetScreenToApzcTransform(const AsyncPanZoomController *aApzc) const;
>    gfx::Matrix4x4 GetApzcToGeckoTransform(const AsyncPanZoomController *aApzc) const;
> +  void GetScrollableLayerGuid(const ScreenPoint& aPoint, ScrollableLayerGuid& aGuid);

Change this to return a ScrollableLayerGuid instead of using an out-param.

::: gfx/layers/apz/test/test_smoothness.html
@@ +9,5 @@
> +
> +  <style>
> +  #scrollbox {
> +    height: 5000px;
> +    overflow: scroll;

Setting scrollbox to overflow:scroll is unnecessary, because it has no content and isn't overflowing. It's the body element that's scrolling here, because scrollbox is so big. "scrollbox" is also probably not the best name for this.

@@ +16,5 @@
> +  </style>
> +  <script type="text/javascript">
> +    var scrollEvents = 100;
> +    var i = 0;
> +    var scrollDiv = document.getElementById("scrollbox");

At the point that this script is run the scrollbox element may not have been added to the DOM yet. I suspect this works by accident but may intermittently fail; it would be better to move this inside sendScrollEvent.

@@ +33,5 @@
> +        var dy = -10; // Negative to scroll down
> +        synthesizeNativeWheelAndWaitForEvent(scrollDiv, x, y, dx, dy);
> +        window.requestAnimationFrame(sendScrollEvent);
> +      } else {
> +        SpecialPowers.popPrefEnv(SimpleTest.finish);

You don't need to call popPrefEnv manually. If you call SimpleTest.finish() it will call SpecialPowers.flushPrefEnv() which pops all the pushed prefs. However you should move the SimpleTest.finish() call to the end of this else block.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +545,5 @@
>  }
>  
>  static bool
> +SampleAPZAnimations(const LayerMetricsWrapper& aLayer,
> +                                             TimeStamp aSampleTime)

Spurious change, remove

@@ +576,5 @@
> +    if (!apzc) {
> +      continue;
> +    }
> +    gfx::Matrix4x4 shadowTransform = aLayer->AsLayerComposite()->GetShadowTransform();
> +    if (shadowTransform.Is2DIntegerTranslation() && !shadowTransform.IsIdentity()) {

Why "Integer"? The shadow transform is often a non-integer 2d translation

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +91,5 @@
>    // another animation frame.
>    enum class TransformsToSkip : uint8_t { NoneOfThem = 0, APZ = 1 };
>    bool TransformShadowTree(TimeStamp aCurrentFrame,
>      TransformsToSkip aSkip = TransformsToSkip::NoneOfThem);
> +  void RecordShadowTransforms(Layer* aLayer);

Group this with the GetFrameUniformity declaration below and add comments for both.

::: gfx/layers/composite/ScrollData.cpp
@@ +32,5 @@
> +    average += Point(std::fabs(movement.x), std::fabs(movement.y));
> +    current = nextTransform;
> +  }
> +
> +  average.x = average.y / (float) length;

typo! Also you should be able to do average = average / (float)length; rather than doing the components individually which avoids the typo problem entirely.

@@ +53,5 @@
> +    double diffX = move.x - average.x;
> +    diffX *= diffX;
> +
> +    double diffY = move.y - average.y;
> +    diffY *= diffY;

Again, I'd rather you did

gfx::Point diff = move - average;
stdDev.x += (diff.x * diff.x);
stdDev.y += (diff.y * diff.y);

to avoid doing individual component operations as much as possible.

@@ +62,5 @@
> +    current = next;
> +  }
> +
> +  stdDev.x = stdDev.x / mTransforms.Length();
> +  stdDev.y = stdDev.y / mTransforms.Length();

Ditto

@@ +85,5 @@
> +double
> +ScrollData::EndTest(ScrollableLayerGuid& aScrollLayer)
> +{
> +  double result = CalculateFrameUniformity(aScrollLayer);
> +  Reset();

Seems odd to me that calling EndTest on a single layer will reset all the data for all the layers. What if you want to measure the uniformity of multiple layers over the same period in time? Reading the data for one layer will make it impossible to read the others. Seems like an API decision that might result in unnecessary rework later.

::: gfx/layers/composite/ScrollData.h
@@ +14,5 @@
> +struct LayerTransforms {
> +  gfx::Point GetAverage();
> +  gfx::Point GetStdDev();
> +
> +  ScrollableLayerGuid mGuid;

Don't need mGuid, it's never read anywhere

@@ +25,5 @@
> +
> +public:
> +  ScrollData() {}
> +
> +  void AddMeasurement(ScrollableLayerGuid& aGuid, gfx::Point aTransform);

const-ref both of these

@@ +27,5 @@
> +  ScrollData() {}
> +
> +  void AddMeasurement(ScrollableLayerGuid& aGuid, gfx::Point aTransform);
> +  void Reset();
> +  double EndTest(ScrollableLayerGuid& aScrollLayer);

const-ref

@@ +28,5 @@
> +
> +  void AddMeasurement(ScrollableLayerGuid& aGuid, gfx::Point aTransform);
> +  void Reset();
> +  double EndTest(ScrollableLayerGuid& aScrollLayer);
> +  protected:

un-indent "protected"

@@ +33,5 @@
> +  ~ScrollData();
> +
> +private:
> +  double CalculateFrameUniformity(ScrollableLayerGuid& aScrollLayerId);
> +  LayerTransforms* GetLayerTransforms(ScrollableLayerGuid& aLayerId);

const-ref both
Comment on attachment 8605957 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1320,5 @@
>  bool
> +CompositorParent::RecvGetFrameUniformity(const ScreenIntPoint& aTouchPoint, float* aFrameUniformity)
> +{
> +  ScrollableLayerGuid scrollLayer;
> +  mApzcTreeManager->GetScrollableLayerGuid(aTouchPoint, scrollLayer);

mApzcTreeManager can be null here. I'm getting a crash on OS X when running as:

mach mochitest-chrome --setpref layers.async-pan-zoom.enabled=true gfx/layers/apz/test/test_smoothness.html

Please guard against that and return -1.
I guess comment 54 is a result of bug 1161407 (i.e. APZ is only enabled when e10s is enabled by default.) So you should probably modify the skip-if for the test to also include !e10s. Unfortunately it means that given the current automation setup, this test will not run in automation at all, even after we turn on APZ. :(

(In reply to Mason Chang [:mchang] from comment #52)
> From the patches in bug 1162064, it looks like a preference wouldn't work
> anyway? APZ might be both e10s/widget dependent, which means we should add a
> check on the actual widget first?

The "widget dependency" part of this is not that relevant here I guess. Assuming APZ + e10s are enabled, the only widgets that don't get an APZCTreeManager are the small widgets created for popups and plugins and such. The widget that corresponds to the test content should have an APZCTreeManager.
Comment on attachment 8605957 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

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

Ok, so here's another problem with the code: multiply layers in the layer tree can share APZCs but have different shadow transforms. So grouping the ScrollData by ScrollableLayerGuid doesn't work, because you get data like this:

staktrace: for { l=2, p=13, v=7 } adding (10,-701)
staktrace: for { l=2, p=13, v=7 } adding (2,-719)
staktrace: for { l=2, p=13, v=7 } adding (10,-711)
staktrace: for { l=2, p=13, v=7 } adding (2,-729)
staktrace: for { l=2, p=13, v=7 } adding (10,-721)
staktrace: for { l=2, p=13, v=7 } adding (2,-739)
staktrace: for { l=2, p=13, v=7 } adding (10,-731)
staktrace: for { l=2, p=13, v=7 } adding (2,-749)
staktrace: for { l=2, p=13, v=7 } adding (10,-741)
staktrace: for { l=2, p=13, v=7 } adding (2,-759)
staktrace: for { l=2, p=13, v=7 } adding (10,-751)
staktrace: for { l=2, p=13, v=7 } adding (2,-769)
staktrace: for { l=2, p=13, v=7 } adding (10,-761)
staktrace: for { l=2, p=13, v=7 } adding (2,-779)
Attachment #8605957 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #55)
> I guess comment 54 is a result of bug 1161407 (i.e. APZ is only enabled when
> e10s is enabled by default.) So you should probably modify the skip-if for
> the test to also include !e10s. Unfortunately it means that given the
> current automation setup, this test will not run in automation at all, even
> after we turn on APZ. :(

Don't some tests run with e10s? https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1c2fbba7e47c

Does M-e10s(BC1) not mean e10s? Since this is a chrome test, this should run?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #56)
> Comment on attachment 8605957 [details] [diff] [review]
> Measure frame uniformity by synthesizing native events
> 
> Review of attachment 8605957 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, so here's another problem with the code: multiply layers in the layer
> tree can share APZCs but have different shadow transforms. So grouping the
> ScrollData by ScrollableLayerGuid doesn't work, because you get data like
> this:
> 
> staktrace: for { l=2, p=13, v=7 } adding (10,-701)
> staktrace: for { l=2, p=13, v=7 } adding (2,-719)
> staktrace: for { l=2, p=13, v=7 } adding (10,-711)
> staktrace: for { l=2, p=13, v=7 } adding (2,-729)
> staktrace: for { l=2, p=13, v=7 } adding (10,-721)
> staktrace: for { l=2, p=13, v=7 } adding (2,-739)
> staktrace: for { l=2, p=13, v=7 } adding (10,-731)
> staktrace: for { l=2, p=13, v=7 } adding (2,-749)
> staktrace: for { l=2, p=13, v=7 } adding (10,-741)
> staktrace: for { l=2, p=13, v=7 } adding (2,-759)
> staktrace: for { l=2, p=13, v=7 } adding (10,-751)
> staktrace: for { l=2, p=13, v=7 } adding (2,-769)
> staktrace: for { l=2, p=13, v=7 } adding (10,-761)
> staktrace: for { l=2, p=13, v=7 } adding (2,-779)

I'll have to rework it. I was also thinking, if we have multiple layers then, what could we do to represent frame uniformity? We could return a list of frame uniformities, but they wouldn't be tied to specific layers :/. Or we return the most non-uniform as our benchmark, which I think is ok.
Flags: needinfo?(bugmail.mozilla)
M-e10s is with e10s, yes, but those suites only run on Linux and your test is only Mac/windows. So if you require both mac/win and e10s it won't get coverage. That will probably change in the future but I'm not sure when.

I think for frame uniformity you could just record the uniformity for all layers and when the test ends, report the uniformity along with some info from the layer like the type and the visible region. Log that information from the test so if it starts failing we can tell from the log hopefully which layer is moving poorly and get a head start on the debugging. Maybe even provide the address of the layer since enabling layer dumping in the test is pretty easy during debugging.
Flags: needinfo?(bugmail.mozilla)
Updated to track all scrolled layers during APZ animations and incorporates the feedback from comment 51. Also passes the layer frame uniformity data from C++ to JS like APZTestData. The test itself checks each layer and ensures that each layer's frame uniformity is ok. It also prints out the layer address so it's easy to test with layers.dump to debug.
Attachment #8605957 - Attachment is obsolete: true
Attachment #8612440 - Flags: review?(bugmail.mozilla)
Comment on attachment 8612440 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

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

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1816,5 @@
> +  /**
> +   * Returns a JSObject which contains a list of frame uniformities
> +   * when the pref gfx.vsync.collect-scroll-data is enabled.
> +   * Every result contains a layer address and a frame uniformity for that layer.
> +   * A negative value indicate an invalid frame uniformity and an error has occured.

A negative value as the frame uniformity? Or a negative value returned from this API?

::: dom/webidl/APZTestData.webidl
@@ +37,5 @@
> +};
> +
> +// A frame uniformity measurement for every scrollable layer
> +dictionary FrameUniformity {
> +  unsigned long layerAddress;

There's a commit hook on .webidl changes, one of the DOM peers needs to review this, I think.

http://hg.mozilla.org/hgcustom/version-control-tools/file/8f6f616a78fd/hghooks/mozhghooks/prevent_webidl_changes.py#l40

::: gfx/2d/BasePoint.h
@@ +59,5 @@
>    }
>  
> +  Sub operator*(const Sub& aPoint) const {
> +    return Sub(x * aPoint.x, y * aPoint.y);
> +  }

Not really sure adding this makes sense generally. Bas would need to review, or you can just get rid of it and do the operation using the individual components in the one spot it's used.

::: gfx/layers/apz/test/chrome.ini
@@ +3,5 @@
> +  apz_test_native_event_utils.js
> +tags = apz-chrome
> +
> +[test_smoothness.html]
> +skip-if = debug || (os != 'mac' && os != 'win') || !e10s # hardware vsync only on win/mac

update comment to say e10s-only because that's where APZ is enabled.

::: gfx/layers/apz/test/test_smoothness.html
@@ +36,5 @@
> +        window.requestAnimationFrame(sendScrollEvent);
> +      } else {
> +        // Locally, with silk and apz + e10s, retina 15" mbp usually get ~1.0 - 1.5
> +        // w/o silk + e10s + apz, I get up to 7. Lower is better.
> +        // Windows, I get ~3. Values are not valid w/o hardware vsync

Can we check hardware sync is enabled using prefs the same way we do APZ?

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +581,5 @@
> +
> +    Matrix transform = shadowTransform.As2D();
> +    if (transform.IsTranslation() && !shadowTransform.IsIdentity()) {
> +      Point translation = transform.GetTranslation();
> +      mFrameUniformityData.RecordTransform(aLayer, translation);

This looks like it will record the same shadow transform n times, where n is the number of APZCs attached to the layer. I think this needs to be moved outside the loop, or just return after executing this part once.

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +77,4 @@
>  public:
>    NS_INLINE_DECL_REFCOUNTING(AsyncCompositionManager)
>  
> +  explicit AsyncCompositionManager(LayerManagerComposite* aManager);

Moving the constructor/destructor into the .cpp file is nice, but unrelated to this bug. I'm fine with leaving it in since I consider it in an improvement but in general try to avoid adding unrelated changes.

@@ +122,5 @@
> +  // will return the frame uniformity for each layer attached to an APZ
> +  void GetFrameUniformity(FrameUniformityData* aFrameUniformityData);
> +
> +  // Records the shadow transform for a given layer due to a scroll
> +  void RecordShadowTransforms(Layer* aLayer);

This function should be private

::: gfx/layers/composite/FrameUniformityData.cpp
@@ +11,5 @@
> +#include "mozilla/TimeStamp.h"
> +#include "nsTArray.h"
> +#include "Units.h"                      // for ScreenPoint, etc
> +#include "mozilla/dom/APZTestDataBinding.h"
> +#include "mozilla/dom/ToJSValue.h"

Alpha-sort includes

@@ +18,5 @@
> +namespace layers {
> +
> +using namespace gfx;
> +
> +gfx::Point

don't need gfx::

@@ +102,5 @@
> +  }
> +
> +  return mFrameTransforms.find(aLayer)->second;
> +}
> +void

newline between functions

::: gfx/layers/composite/FrameUniformityData.h
@@ +7,5 @@
> +#define mozilla_layers_FrameUniformityData_h_
> +
> +#include "nsRefPtr.h"
> +#include "ipc/IPCMessageUtils.h"
> +#include "js/TypeDecls.h"

Move nsRefPtr.h to the bottom for alphabetical goodness.

@@ +24,5 @@
> +  nsAutoTArray<gfx::Point, 300> mTransforms;
> +};
> +
> +class FrameUniformityData {
> +  friend struct IPC::ParamTraits<FrameUniformityData>;

I'd like to split this class into two classes:

- FrameUniformityData, which just contains the std::map<uintptr_t,float> mUniformites. This will get passed around over IPC and is accessible from the LayerManager API. (This could even just be a typedef for the std::map, but it's fine to leave it as a class too)
- FrameUniformityCalculator (or something simialr) which contains everything else in this class. The EndTest function will still produce a FrameUniformityData. The AsyncCompositionManager will hold a FrameUniformityCalculator instead of a FrameUniformityData

That way we avoid the ickiness of only serializing some of the data in this class over the IPC channel, and avoiding exposure of the rest of the class to places like DOMWindowUtils.

@@ +30,5 @@
> +public:
> +  FrameUniformityData() {}
> +  ~FrameUniformityData();
> +
> +  void RecordTransform(Layer* aLayer, const gfx::Point aTransform);

make this a const gfx::Point&

::: gfx/layers/ipc/CompositorParent.cpp
@@ +1715,5 @@
>  
> +  virtual bool RecvGetFrameUniformity(FrameUniformityData* aOutData) override
> +  {
> +    // Don't support calculating frame uniformity on the child process and
> +    // this is just a stub for now.

MOZ_ASSERT(false)?
Attachment #8612440 - Flags: review?(bugmail.mozilla) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #61)
> ::: gfx/2d/BasePoint.h
> @@ +59,5 @@
> >    }
> >  
> > +  Sub operator*(const Sub& aPoint) const {
> > +    return Sub(x * aPoint.x, y * aPoint.y);
> > +  }
> 
> Not really sure adding this makes sense generally. Bas would need to review,
> or you can just get rid of it and do the operation using the individual
> components in the one spot it's used.

I'd prefer not adding this - I think multiplying two points to yield another point isn't a meaningful operation in general. For specialized used cases like yours (calculating a standard deviation), I think it's better to operate on the component manually as Kats suggests.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #61)
> Comment on attachment 8612440 [details] [diff] [review]
> Measure frame uniformity by synthesizing native events
> 
> Review of attachment 8612440 [details] [diff] [review]:
> -----------------------------------------------------------------
 
> ::: dom/webidl/APZTestData.webidl
> @@ +37,5 @@
> > +};
> > +
> > +// A frame uniformity measurement for every scrollable layer
> > +dictionary FrameUniformity {
> > +  unsigned long layerAddress;
> 
> There's a commit hook on .webidl changes, one of the DOM peers needs to
> review this, I think.
> 
> http://hg.mozilla.org/hgcustom/version-control-tools/file/8f6f616a78fd/
> hghooks/mozhghooks/prevent_webidl_changes.py#l40

I'll ask a DOM peer to review once you're done reviewing.

> ::: gfx/2d/BasePoint.h
> @@ +59,5 @@
> >    }
> >  
> > +  Sub operator*(const Sub& aPoint) const {
> > +    return Sub(x * aPoint.x, y * aPoint.y);
> > +  }
> 
> Not really sure adding this makes sense generally. Bas would need to review,
> or you can just get rid of it and do the operation using the individual
> components in the one spot it's used.

Deleted this part and just used individual components for this case.
 
> @@ +24,5 @@
> > +  nsAutoTArray<gfx::Point, 300> mTransforms;
> > +};
> > +
> > +class FrameUniformityData {
> > +  friend struct IPC::ParamTraits<FrameUniformityData>;
> 
> I'd like to split this class into two classes:
> 
> - FrameUniformityData, which just contains the std::map<uintptr_t,float>
> mUniformites. This will get passed around over IPC and is accessible from
> the LayerManager API. (This could even just be a typedef for the std::map,
> but it's fine to leave it as a class too)
> - FrameUniformityCalculator (or something simialr) which contains everything
> else in this class. The EndTest function will still produce a
> FrameUniformityData. The AsyncCompositionManager will hold a
> FrameUniformityCalculator instead of a FrameUniformityData
> 
> That way we avoid the ickiness of only serializing some of the data in this
> class over the IPC channel, and avoiding exposure of the rest of the class
> to places like DOMWindowUtils.

Use FrameUniformityData which contains the std::map<uintptr_t,float> and the logic to convert to JS. Renamed the FrameUniformityCalculator to LayerTransformRecorder.

Updated to fix the other comments.
Attachment #8612440 - Attachment is obsolete: true
Attachment #8613181 - Flags: review?(bugmail.mozilla)
Comment on attachment 8613181 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

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

Looks good. I'm building locally just to verify and see what kind of numbers I get on my machine, will r+ if all goes well.

::: gfx/layers/composite/AsyncCompositionManager.h
@@ +117,5 @@
>    // particular document.
>    bool IsFirstPaint() { return mIsFirstPaint; }
>  
> +  // After enough calls to RecordShadowTransforms, GetFrameUniformity
> +  // will return the frame uniformity for each layer attached to an APZ

What does "enough" mean here?

@@ +187,5 @@
>     * since ResolveRefLayers was called.
>     */
>    void DetachRefLayers();
>  
> +  // Records the shadow transform for a given layer due to a scroll

This comment isn't really correct. Should be more like "Records the shadow transforms for the tree of layers rooted at the given layer."

::: gfx/layers/composite/FrameUniformityData.cpp
@@ +11,5 @@
> +#include "nsTArray.h"
> +#include "Units.h"
> +#include "mozilla/TimeStamp.h"
> +#include "mozilla/dom/APZTestDataBinding.h"
> +#include "mozilla/dom/ToJSValue.h"

Sort includes
Attachment #8613181 - Flags: review?(bugmail.mozilla) → review+
Added feedback from comment 64.

@mrbkap - Can you please review the webidl bindings as they need a DOM peer to review. Thanks!
Attachment #8613181 - Attachment is obsolete: true
Attachment #8614273 - Flags: review?(mrbkap)
Comment on attachment 8614273 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

Carrying r+ from :kats.
Attachment #8614273 - Flags: review+
Comment on attachment 8614273 [details] [diff] [review]
Measure frame uniformity by synthesizing native events

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

I have a couple of nits that you can take or ignore as you want.

::: gfx/layers/composite/FrameUniformityData.cpp
@@ +135,5 @@
> +  dom::FrameUniformityResults results;
> +  dom::Sequence<dom::FrameUniformity>& layers = results.mLayerUniformities.Construct();
> +
> +  std::map<uintptr_t,float>::iterator iter;
> +  for (iter = mUniformities.begin(); iter != mUniformities.end(); ++iter) {

Isn't this the canonical use for 'auto'?

@@ +140,5 @@
> +    uintptr_t layerAddr = iter->first;
> +    float uniformity = iter->second;
> +
> +    layers.AppendElement();
> +    dom::FrameUniformity& entry = layers.LastElement();

This could be: dom::FrameUniformity& entry = *layers.AppendElement();

@@ +147,5 @@
> +    entry.mFrameUniformity.Construct() = uniformity;
> +  }
> +
> +  bool result = dom::ToJSValue(aContext, results, aOutValue);
> +  return result;

Nit: no need for |result|.
Attachment #8614273 - Flags: review?(mrbkap) → review+
https://hg.mozilla.org/mozilla-central/rev/a9cff1d9e7c6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
[Tracking Requested - why for this release]:

Mason, could you share how to generate benchmark? we could work with raptor team to report data on performance dashboard.
Keywords: perf
Whiteboard: [perf-wanted]
Flags: needinfo?(mchang)
Hi Bobby, I added the documentation here - https://wiki.mozilla.org/Project_Silk#Running_the_mochitest
Flags: needinfo?(mchang)
You need to log in before you can comment on or make changes to this bug.