814437 - Talos Regression tcheck, tcheck2 and tcheck3 increase 400% on Android Nov 22

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Geoff Brown [:gbrown]]

Message: 1
Date: Thu, 22 Nov 2012 02:40:32 -0000
From: [e-mail]
To: [e-mail]
Subject: Talos Regression Robocop Checkerboarding No Snapshot
        Benchmark        increase 321% on Android 2.2 (Native) Mozilla-Inbound
Message-ID:
        <[e-mail]>
Content-Type: text/plain; charset="us-ascii"

Regression Robocop Checkerboarding No Snapshot Benchmark increase 321% on Android 2.2 (Native) Mozilla-Inbound
-----------------------------------------------------------------------------------------------------------------
    Previous: avg 2129.610 stddev 1879.787 of 30 runs up to revision c4e31e87a072
    New     : avg 8958.056 stddev 349.318 of 5 runs since revision c4d013240eac
    Change  : +6828.446 (321% / z=3.633)
    Graph   : http://mzl.la/Th6OpP

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c4e31e87a072&tochange=c4d013240eac

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/f7f8011950c9
    : Terrence Cole <[e-mail]> - Bug 811911 - Allow UTF-8 output from the SpiderMonkey shell; r=Norbert
    : http://bugzilla.mozilla.org/show_bug.cgi?id=811911

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/bfd5f652e5f0
    : Mario Alvarado [:marioalv] <[e-mail]> - Bug 806699 - Port browser_privatebrowsing_ui.js to the new per-window PB APIs; r=ehsan
    : http://bugzilla.mozilla.org/show_bug.cgi?id=806699

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/7b0171b51aa3
    : Chris Lord <[e-mail]> - Bug 783368 - Add critical display port content property. r=roc

Add a property to represent a sub-rectangle of the display port that is
considered 'critical' to render correctly.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/a79d36c9174b
    : Chris Lord <[e-mail]> - Bug 783368 - Prefer the critical display port, when set, in basic tiled layers. r=bgirard

If a critical display port is set, only validate content within it when using
basic tiled layers.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/729f9f0d437f
    : Chris Lord <[e-mail]> - Bug 783368 - Add resolution to TiledLayerBuffer. r=bgirard

Add the concept of resolution to TiledLayerBuffer and add support for it in
BasicTiledThebesLayer and TiledThebesLayerOGL.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/13917d4b99b8
    : Chris Lord <[e-mail]> - Bug 783368 - Disable ReusableTileStoreOGL by default. r=blassey

Add layers.reuse-invalid-tiles to control use of the reusable tile store and
disable it by default.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/f74b22565be0
    : Chris Lord <[e-mail]> - Bug 783368 - Add a low precision buffer to tiled thebes layers. r=bgirard,kats

If there is a critical display port set, render the area outside it into a
lower precision buffer.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/7d09617883c7
    : Chris Lord <[e-mail]> - Bug 783368 - Don't duplicate high precision rendering in low precision region. r=bgirard

Don't render valid high precision region in the low precision buffer.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/3f1b3e8b8ac7
    : Chris Lord <[e-mail]> - Bug 783368 - Send the correct display port when rendering low precision. r=kats,bgirard

When doing a low precision update, send the display-port instead of the
critical display port so that more appropriate cancelling decisions can be
made.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/808c7d0aedec
    : Chris Lord <[e-mail]> - Bug 783368 - Correct the height calculation in ProgressiveUpdateData. r=kats
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/d014526b795e
    : Chris Lord <[e-mail]> - Bug 783368 - Fix progressive tile update coherency issues. r=bgirard

Fix some progressive tile updating coherency issues caused by aborting at
inopportune times and tile draw ordering.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/e93e08cd1d4c
    : Chris Lord <[e-mail]> - Bug 783368 - Uncouple low precision updates from standard. r=bgirard

Allow low precision updates to happen regardless of the state of the unscaled
tile buffer.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/c4d013240eac
    : Chris Lord <[e-mail]> - Bug 783368 - Add a pref to control low precision tile rendering. r=bgirard

Add pref 'layers.low-precision-buffer', enabled on mobile/android.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=783368 - Add a low res tile cache to TiledThebesLayer
  * http://bugzilla.mozilla.org/show_bug.cgi?id=806699 - Port browser_privatebrowsing_ui.js to the new per-window PB APIs
  * http://bugzilla.mozilla.org/show_bug.cgi?id=811911 - Regression: Unicode option -U removed from js shell


------------------------------

Message: 2
Date: Thu, 22 Nov 2012 03:01:12 -0000
From: [e-mail]
To: [e-mail]
Subject: Talos Regression Robocop Checkerboarding Real User
        Benchmark        increase 396% on Android 2.2 (Native) Mozilla-Inbound
Message-ID:
        <[e-mail]>
Content-Type: text/plain; charset="us-ascii"

Regression Robocop Checkerboarding Real User Benchmark increase 396% on Android 2.2 (Native) Mozilla-Inbound
---------------------------------------------------------------------------------------------------------------
    Previous: avg 1853.692 stddev 1404.591 of 30 runs up to revision f7f8011950c9
    New     : avg 9194.874 stddev 282.315 of 5 runs since revision d00a4624190f
    Change  : +7341.182 (396% / z=5.227)
    Graph   : http://mzl.la/Th8D6c

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f7f8011950c9&tochange=d00a4624190f

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/bfd5f652e5f0
    : Mario Alvarado [:marioalv] <[e-mail]> - Bug 806699 - Port browser_privatebrowsing_ui.js to the new per-window PB APIs; r=ehsan
    : http://bugzilla.mozilla.org/show_bug.cgi?id=806699

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/7b0171b51aa3
    : Chris Lord <[e-mail]> - Bug 783368 - Add critical display port content property. r=roc

Add a property to represent a sub-rectangle of the display port that is
considered 'critical' to render correctly.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/a79d36c9174b
    : Chris Lord <[e-mail]> - Bug 783368 - Prefer the critical display port, when set, in basic tiled layers. r=bgirard

If a critical display port is set, only validate content within it when using
basic tiled layers.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/729f9f0d437f
    : Chris Lord <[e-mail]> - Bug 783368 - Add resolution to TiledLayerBuffer. r=bgirard

Add the concept of resolution to TiledLayerBuffer and add support for it in
BasicTiledThebesLayer and TiledThebesLayerOGL.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/13917d4b99b8
    : Chris Lord <[e-mail]> - Bug 783368 - Disable ReusableTileStoreOGL by default. r=blassey

Add layers.reuse-invalid-tiles to control use of the reusable tile store and
disable it by default.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/f74b22565be0
    : Chris Lord <[e-mail]> - Bug 783368 - Add a low precision buffer to tiled thebes layers. r=bgirard,kats

If there is a critical display port set, render the area outside it into a
lower precision buffer.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/7d09617883c7
    : Chris Lord <[e-mail]> - Bug 783368 - Don't duplicate high precision rendering in low precision region. r=bgirard

Don't render valid high precision region in the low precision buffer.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/3f1b3e8b8ac7
    : Chris Lord <[e-mail]> - Bug 783368 - Send the correct display port when rendering low precision. r=kats,bgirard

When doing a low precision update, send the display-port instead of the
critical display port so that more appropriate cancelling decisions can be
made.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/808c7d0aedec
    : Chris Lord <[e-mail]> - Bug 783368 - Correct the height calculation in ProgressiveUpdateData. r=kats
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/d014526b795e
    : Chris Lord <[e-mail]> - Bug 783368 - Fix progressive tile update coherency issues. r=bgirard

Fix some progressive tile updating coherency issues caused by aborting at
inopportune times and tile draw ordering.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/e93e08cd1d4c
    : Chris Lord <[e-mail]> - Bug 783368 - Uncouple low precision updates from standard. r=bgirard

Allow low precision updates to happen regardless of the state of the unscaled
tile buffer.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/c4d013240eac
    : Chris Lord <[e-mail]> - Bug 783368 - Add a pref to control low precision tile rendering. r=bgirard

Add pref 'layers.low-precision-buffer', enabled on mobile/android.
    : http://bugzilla.mozilla.org/show_bug.cgi?id=783368

  * http://hg.mozilla.org/integration/mozilla-inbound/rev/d00a4624190f
    : Kartikaya Gupta <[e-mail]> - Bug 813204 - Don't send multiple hasTouchListener events for the same document. r=wesj
    : http://bugzilla.mozilla.org/show_bug.cgi?id=813204

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=783368 - Add a low res tile cache to TiledThebesLayer
  * http://bugzilla.mozilla.org/show_bug.cgi?id=806699 - Port browser_privatebrowsing_ui.js to the new per-window PB APIs
  * http://bugzilla.mozilla.org/show_bug.cgi?id=813204 - Loading the economist is hellaslow

Comment 1

[Geoff Brown [:gbrown]]

It looks like bug 783368 caused this (slight chance of bug 806699 which landed just before and did not trigger rck2/rck3). 

Bug 783368 has a try run which also shows the increase in rck2 and rck3: https://tbpl.mozilla.org/?tree=Try&rev=badff86f29ca

Comment 2

[Chris Lord [:cwiiis]]

Argh, once again, our measurement needs to change - the 'completeness' will be about a quarter of what it should be due to the valid region not counting the low precision buffer (explaining the ~400% - it'll be less because it's unlikely the width will ever reach the full size). Will fix.

Comment 3

[Chris Lord [:cwiiis]]

Actually, I'm wrong, this ought to be the correct(-ish) value. What has changed is that the calculation of it is much more expensive, as for every layer it's now guaranteed that it'll have to do at least 4 transforms, and likely more.

I would expect the value to go down given it doesn't count low-res and it isn't free to draw low-res, but this amount seems a bit much. The calculation of the value may be having an effect too, but again, this seems a bit much.

I'm not sure of the behaviour of talos, but the hit for low-res tiles should be quite small after the initial render of the page - this initial render may be adversely affecting the result if the test doesn't run for that long.

There's also the possibility that the calculation is wrong...

This will require a bit of investigation, but I'd urge us not to back out the patches - the user experience, on the phones I've tested at least, is much better.

Comment 4

[Chris Lord [:cwiiis]]

I do feel that if we're spending time drawing the low res region, it should count towards checkerboard - I'm writing a patch that will make it count 0.5x the checkerboarding value (this seems fair - a whole screen of low-res is a whole lot better than a blank screen, and is often readable, especially on high-ppi screens).

I don't think this will recover these values completely though, I'll have to look at other performance-saving measures.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Count low precision rendering towards checkerboard

Count low-precision rendering 0.5x towards the checkerboarding value.

Still waiting on a try push for talos results:
https://tbpl.mozilla.org/?tree=Try&rev=db95149913a3

I expect/hope that this would get us around half-way back to the old values. That try build also has the patch to fix aborting (a try push of that alone here, in case it makes a significant difference: https://tbpl.mozilla.org/?tree=Try&rev=e00bb2735c33)

Comment 6

[Chris Lord [:cwiiis]]

The figures haven't improved as much as I expected - having suspicions that the integrity calculation may be wrong. Will check this tomorrow. Also, we probably shouldn't draw stale low-res content when there's future draws to happen - I'll do this too.

Comment 7

[Geoff Brown [:gbrown]]

It's more subtle and less consistent, but a similar talos regression was detected for tcheck, and it looks to me like it goes back to the same check-in for bug 783368:

Message: 2
Date: Fri, 23 Nov 2012 20:01:50 -0000
From: [e-mail]
To: [e-mail]
Subject: Talos Regression Robocop Checkerboarding Benchmark
        increase 389%        on Android 2.2 (Native) mobile
Message-ID:
        <[e-mail]>
Content-Type: text/plain; charset="us-ascii"

Regression Robocop Checkerboarding Benchmark increase 389% on Android 2.2 (Native) mobile
--------------------------------------------------------------------------------------------
    Previous: avg 9.061 stddev 8.804 of 30 runs up to revision c27455415eac
    New     : avg 44.284 stddev 6.238 of 5 runs since revision d8e4f06198dc
    Change  : +35.223 (389% / z=4.001)
    Graph   : http://mzl.la/UPqm1y

Changeset range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c27455415eac&tochange=d8e4f06198dc

Changesets:
  * http://hg.mozilla.org/mozilla-central/rev/d8e4f06198dc
    : Mike Hommey <[e-mail]> - Bug 799295 - Work around gcc 4.5 miscompilation of CanEncodeInfoInHeader by always inlining it
    : http://bugzilla.mozilla.org/show_bug.cgi?id=799295

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=799295 - v8 crashing most runs on Linux32 PGO, in js::gc::MarkKind, on Gecko 18 and up

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 684491 [details] [diff] [review]
Part 1 - Count low precision rendering towards checkerboard

I'm removing the review flag until I've confirmed that the calculations in ComputeRenderIntegrity are correct.

Comment 9

[Chris Lord [:cwiiis]]

Ah, I've found out the measurement *is* incorrect - it measures too low when there's a screen full of low-precision checkerboarding. Will fix.

Comment 10

[Chris Lord [:cwiiis]]

In fact, I think it just measures too low (or too high, rather), a lot of the time, not just for low-precision... So our numbers are better than we've been measuring anyway...

Comment 11

[Chris Lord [:cwiiis]]

Only with low-precision/an enlarged displayport is the value wrong.

The problem is that we calculate the checkerboard by first calculating how much of the page area is covered by the displayport, then multiplying that by the integrity of what's rendered inside it.

With an enlarged displayport, that first calculation is wrong, so any time we checkerboard, we record much higher values of checkerboarding than are actually happening (which is why the values recorded in bug 814437 are so high even when just enlarging the displayport and doing nothing else).

Hopefully we can just use the composition bounds stored in FrameMetrics to do the same thing correctly on Gecko side.

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 684491 [details] [diff] [review]
Part 1 - Count low precision rendering towards checkerboard

This patch is fine.

Comment 13

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Fix checkerboard measurement when using a critical displayport

Comment 14

[Chris Lord [:cwiiis]]

Try build with these patches and some of the work in bug 814864 applied:

https://tbpl.mozilla.org/?tree=Try&rev=614d6d9534b4

Comment 15

[Chris Lord [:cwiiis]]

Comment on attachment 685153 [details] [diff] [review]
Part 2 - Fix checkerboard measurement when using a critical displayport

Removing r? as try shows I can't possibly have gotten this right (the numbers have increased significantly). Likely an incorrect/missing 1-something somewhere...

Comment 16

[Chris Lord [:cwiiis]]

Comment on attachment 685153 [details] [diff] [review]
Part 2 - Fix checkerboard measurement when using a critical displayport

Sorry for churn, actually think the previous code may have been wrong and would like to get this reviewed. If I discover any problems, will update.

Comment 17

[Chris Lord [:cwiiis]]

I've run with this patch and all the numbers look correct (and the code looks correct to me too) - I'm very dubious of the pre-patch line:

checkerboard += (1.0 - checkerboard) * (1.0 - GeckoAppShell.computeRenderIntegrity());

It seems like that should have been 'checkerboard = ...', not 'checkerboard += ...'

Here's a try push with low precision rendering disabled (that I've shown in bug 814864 replicated the performance before any low precision/critical displayport work was committed) - the numbers from this should be a good indicator if the previous code was correct or not.

https://tbpl.mozilla.org/?tree=Try&rev=311da4b2f0e0

Comment 18

[Chris Lord [:cwiiis]]

ok, results are saying that performance was actually better than we were recording... So at this point, I'm slightly at a loss as to what's going on... I'll investigate more tomorrow.

Comment 19

[Chris Lord [:cwiiis]]

I don't know how rck calculates its results, but here are the results from the logs of rck2, in terms of frames rendered. I've put *'s after values I'd consider to be winning.

With low-pres|Without
-------------|-------------
29.0/33      |32.0/32*
36.690647/37*|31.0/31
36.0/36*     |29.975174/30
35.75746/36* |32.0/32
22.0/22      |29.0/29*
34.436714/38 |36.0/36*
20.0/30      |24.0/24*
23.0/23      |25.0/25*
42.0/42*     |26.0/26
31.0/31*     |26.0/26
24.56012/35  |27.829872/32*
23.0/31*     |19.837929/23
29.0/29      |29.0/29
35.0/35*     |32.0/32
33.0/33*     |25.0/25
---------------------------
8 wins       |6 wins

So on the whole, not a huge amount of difference in synthetic performance numbers, but for the most part, low precision on renders more frames (which would suggest a better framerate), and doesn't ever lose by a significant amount (apart from perhaps tests 5 and 7).

I don't understand how these results are calculated from these values :/

Comment 20

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #19)
> amount (apart from perhaps tests 5 and 7).

*seconds 5 and 7.

Comment 21

[Chris Lord [:cwiiis]]

I've been discussing with kats how rck works. The bad test results in rck2/3, going on these frame numbers, could indicate that there are more frames rendered (due to higher fps), but that aren't 100% complete.

kats suggested establishing a new way of measuring checkerboarding, such as checkerboard/time rather than the current checkerboard/frame. I think this would normalise these results (and these test results in general), but unfortunately, we won't be able to compare with our previous results.

Comment 22

[Geoff Brown [:gbrown]]

There's definitely value to having consistent tests that can be compared over long periods of time, but sometimes it's just not possible. I think it's more important to have a sensible test that gives us a relevant performance measure -- change the checkerboard measurement if you have to.

Comment 23

[Benoit Girard (:BenWa)]

Comment on attachment 684491 [details] [diff] [review]
Part 1 - Count low precision rendering towards checkerboard

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

r+ with this changed or explanation why you think it's better this way.

::: gfx/layers/Layers.h
@@ +1145,5 @@
>    /**
>     * Can be used anytime
>     */
>    const nsIntRegion& GetValidRegion() const { return mValidRegion; }
> +  virtual const nsIntRegion& GetValidLowPrecisionRegion() const { return mValidRegion; }

I think I would prefer to keep this away from Layers.h. This implies that we're exposing to the users of layers that we do low res drawing which I don't think belongs in this patch. If that's something we want to do later we should design it properly.

Instead we could use try to convert it to a shadow and then a tile composer and introduce the method on tile composer:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.h#549

Feel free to disagree. There's might be another alternative as well.

Comment 24

[Benoit Girard (:BenWa)]

Comment on attachment 685153 [details] [diff] [review]
Part 2 - Fix checkerboard measurement when using a critical displayport

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

r+ with this fix.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1709,5 @@
>        lowPrecisionIntegrity = GetRegionArea(lowPrecisionScreenRegion) / screenArea;
>      }
>  
> +    return ((highPrecisionIntegrity * highPrecisionMultiplier) +
> +            (lowPrecisionIntegrity * lowPrecisionMultiplier)) / 2.f;

2.f needs to be (highPrecisionMultiplier+lowPrecisionMultiplier)

Comment 25

[Benoit Girard (:BenWa)]

(In reply to Chris Lord [:cwiiis] from comment #21)
> kats suggested establishing a new way of measuring checkerboarding, such as
> checkerboard/time rather than the current checkerboard/frame. I think this
> would normalise these results (and these test results in general), but
> unfortunately, we won't be able to compare with our previous results.

Yes, we since our frame is variable we need to include the time a frame stays on screen in the calculation. I agree with gbrown argument about why it ok to relist this benchmark.

Comment 26

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Record average checkerboard over time

This patch makes checkerboard average out per time and ends up with a fractional value between 0 and 100, where 100 is the worst possible score (entire checkerboard all the time) and 0 is the best (no checkerboard at all). Frame-rate and run-length have no effect on score, these should be tested elsewhere.

This shows the real problem occuring here - none of the rck tests actually tax us all that much and low precision tiles take work but are worth less than high precision, so we just end up with worse results. I expect results would be better if the run-time of these tests was increased, or it panned faster/more. rck is the exception, where I'm guessing the page taxes us so little that there is just one instance of checkerboarding, and in that one instance with low precision tiles, it penalises us less.

The problem with low precision tiles is that we're trading much improved performance in our worst case for worse performance in our best case - this will hopefully be solved by the suggestions made in bug 814864.

Results without low precision: https://tbpl.mozilla.org/?tree=Try&rev=7e801d0b752d (0.07, 0.81, 0.74)
Results with low precision: https://tbpl.mozilla.org/?tree=Try&rev=7eb0257447c7 (0.0, 12.75, 13.29)

So we're barely checkerboarding in any of these tests without low precsion. I think we should take this patch as these results are much easier to read and understand imho.

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 685609 [details] [diff] [review]
Part 3 - Record average checkerboard over time

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

I'm fine with the overall concept but there's a few things with the implementation and data calculations that could be improved.

::: mobile/android/base/gfx/PanningPerfAPI.java
@@ +47,5 @@
>          }
> +        if (mRecordingCheckerboard) {
> +            Log.e(LOGTAG, "Error: startFrameTimeRecording() called while recording checkerboarding!");
> +            return;
> +        }

I would prefer folding the two if conditions into a single check (i.e. mRecordingFrames || mRecordingCheckerboard) and making the error message a little more generic.

@@ +81,5 @@
>          }
> +        if (mRecordingFrames) {
> +            Log.e(LOGTAG, "Error: startCheckerboardRecording() called while recording frame times!");
> +            return;
> +        }

Same with the folding of the if-conditions. You could even pull them out into a helper method since the code is duplicated but it's not really needed yet.

@@ +105,5 @@
> +        float totalTime = mFrameTimes.get(mFrameTimes.size()-1) - mFrameTimes.get(0);
> +        for (int i = 1; i < mCheckerboardAmounts.size(); i++) {
> +          mCheckerboardAmounts.set(i - 1,
> +            ((mCheckerboardAmounts.get(i) + mCheckerboardAmounts.get(i-1)) / 2.0f) *
> +            (mFrameTimes.get(i) - mFrameTimes.get(i-1))/totalTime);

Please use some well-named temp variables for the intermediate expressions so that this is easier to read.

Also since we record the start time in mCheckerboardStartTime, I think you can adjust totalTime to use that instead of mFrameTimes.get(0), and adjust the loop to iterate from i=0. I'm not sure why you're taking the average of the two checkerboard amounts but that can go away as well; you'd basically be left with the equivalent of this:
  mCheckerboardAmounts[i] *= mFrameTimes[i] - (i == 0 ? 0 : mFrameTimes[i-1])

(assuming you make the change to what is stored in mFrameTimes as noted below).

@@ +115,5 @@
>  
>      public static void recordCheckerboard(float amount) {
>          // this will be called often, so try to make it as quick as possible
>          if (mRecordingCheckerboard) {
> +            mFrameTimes.add(SystemClock.uptimeMillis());

For consistency with usage of mFrameTimes in the frame-time-recording code path, I would prefer that you changed this to "SystemClock.uptimeMillis() - mCheckerboardStartTime". Also this will put a use to mCheckerboardStartTime which was previously completely unnecessary (shame on me for not catching that bit of copypasta during review).

Comment 28

[Kartikaya Gupta (email:[e-mail])]

(In reply to Chris Lord [:cwiiis] from comment #26)
> This shows the real problem occuring here - none of the rck tests actually
> tax us all that much and low precision tiles take work but are worth less
> than high precision, so we just end up with worse results. I expect results
> would be better if the run-time of these tests was increased, or it panned
> faster/more. rck is the exception, where I'm guessing the page taxes us so
> little that there is just one instance of checkerboarding, and in that one
> instance with low precision tiles, it penalises us less.
> 
> The problem with low precision tiles is that we're trading much improved
> performance in our worst case for worse performance in our best case - this
> will hopefully be solved by the suggestions made in bug 814864.

Well that's good to know. Are we sure we want to be making this trade-off? Also do we want to add some more complicated pages to the test suite?

Comment 29

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Record average checkerboard over time v2

Make all the suggested changes. Sorry for the generally poor quality of the original patch, I think I need to get some early nights :)

Re the trade-off, no, I don't think we want to just wholesale make that trade-off - I've been working on this in bug 814864 and I think I've reached a reasonable compromise, though again, there are cases before that will measure worse (similarly, there are cases that will measure better - rck and rp among them).

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 686193 [details] [diff] [review]
Part 3 - Record average checkerboard over time v2

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

::: mobile/android/base/gfx/PanningPerfAPI.java
@@ +98,5 @@
> +        float totalTime = mFrameTimes.get(mFrameTimes.size() - 1);
> +        for (int i = 0; i < mCheckerboardAmounts.size(); i++) {
> +          long elapsedTime = mFrameTimes.get(i) - lastTime;
> +          mCheckerboardAmounts.set(i, mCheckerboardAmounts.get(i) * elapsedTime / totalTime);
> +          lastTime = mFrameTimes.get(i);

4-space indent here. Rest looks great, thanks! :)

Comment 31

[Chris Lord [:cwiiis]]

Pushed to inbound, but forgot to address BenWa's patch comments - will address in a follow-up patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/67720d8757c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c67357c751
https://hg.mozilla.org/integration/mozilla-inbound/rev/f039a331337f

I think we've agreed to accept a new base-line, so once the follow-up patch is in, this bug can be marked as fixed.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67720d8757c8
https://hg.mozilla.org/mozilla-central/rev/78c67357c751
https://hg.mozilla.org/mozilla-central/rev/f039a331337f

Comment 33

[Chris Lord [:cwiiis]]

Attachment: Part 4 - followup

This is the follow-up I missed when I pushed these patches (:()

Comment 34

[Chris Lord [:cwiiis]]

Attachment: Part 4 - followup

Try actually attaching the patch this time.

Comment 35

[Chris Lord [:cwiiis]]

Comment on attachment 687055 [details] [diff] [review]
Part 4 - followup

Removing r? as there are a couple more things I need to sort out.

Comment 36

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #24)
> Comment on attachment 685153 [details] [diff] [review]
> Part 2 - Fix checkerboard measurement when using a critical displayport
> 
> Review of attachment 685153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with this fix.
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +1709,5 @@
> >        lowPrecisionIntegrity = GetRegionArea(lowPrecisionScreenRegion) / screenArea;
> >      }
> >  
> > +    return ((highPrecisionIntegrity * highPrecisionMultiplier) +
> > +            (lowPrecisionIntegrity * lowPrecisionMultiplier)) / 2.f;
> 
> 2.f needs to be (highPrecisionMultiplier+lowPrecisionMultiplier)

Just a note, this should be 2.f - we're averaging out these two values, and if the multipliers don't add up to 2.f, we want a value less than 1. (i.e. if low and high multipliers are 0.5, the largest number that should be returned is 0.5)

Comment 37

[Chris Lord [:cwiiis]]

Attachment: Part 4 - followup v2

This version is careful to not consider overscroll to be an incompletely rendered area of the screen. Tested at various zoom levels/overscrolls to make sure the transform is right (it seems to be). It's also careful to make sure that if there isn't low precision rendering happening, it doesn't affect the total.

Comment 38

[Chris Lord [:cwiiis]]

Once part 4 reaches m-c, this bug should be complete.

Comment 39

[Benoit Girard (:BenWa)]

Comment on attachment 687164 [details] [diff] [review]
Part 4 - followup v2

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

::: gfx/layers/TiledLayerBuffer.h
@@ +193,5 @@
>    virtual void MemoryPressure() = 0;
> +
> +  /**
> +   * If some area of the buffer is being rendered at a lower precision, returns
> +   * the region of that area.

Please add. If it is not ___ is return.

Comment 40

[Chris Lord [:cwiiis]]

Thanks for the review, pushed to inbound with amended comment:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5368aced79b3

For reference, talos results with this fixed:

Without low precision: https://tbpl.mozilla.org/?tree=Try&rev=88a24356979d
With low precision: https://tbpl.mozilla.org/?tree=Try&rev=3b7edb40dc20

These numbers are now much more in line with what I'd expect :) There's still room for improvement and I think it'd be worth adding a harder checkerboard test (a long, complex page like desktop engadget.com, or a page that triggers slow rendering like taskjs.org), but that can be dealt with some other time.

Comment 41

[(no longer active)]

Backed out because this breaks Android builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f4db7426eee

Comment 42

[Chris Lord [:cwiiis]]

Botched conflict resolution, pushed again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b666ab33a92

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b666ab33a92

Comment 44

[Kartikaya Gupta (email:[e-mail])]

Note: this work was uplifted to 19 as part of bug 817575.