1208636 - Adjust displayport size based on available system memory

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #1208554 +++

On systems with more memory, we can increase the skate displayport multiplier to prevent checkerboarding. Find a few systems, tweak the setting, and find an appropriate desktop multiplier.

Comment 1

[Botond Ballo [:botond]]

When doing this, we should keep in mind that allowing Firefox to hog a significant amount of the system memory in the name of scrolling performance isn't always desirable. 

For example, I often run a VM that takes up about half of my system's memory, and thus requires about half of my system's memory to be free to start up. If Firefox running on the host system is sufficiently memory-hungry that it doesn't leave half of my system's memory free and I can't start up my VM, I'm not a happy user.

Comment 2

[Mason Chang [Inactive] [:mchang]]

(In reply to Botond Ballo [:botond] from comment #1)
> When doing this, we should keep in mind that allowing Firefox to hog a
> significant amount of the system memory in the name of scrolling performance
> isn't always desirable. 
> 
> For example, I often run a VM that takes up about half of my system's
> memory, and thus requires about half of my system's memory to be free to
> start up. If Firefox running on the host system is sufficiently
> memory-hungry that it doesn't leave half of my system's memory free and I
> can't start up my VM, I'm not a happy user.

That's true, but I'm less worried about it in this case. For example, this is mostly concerned about the skating performance, which means you're actively looking at Firefox and scrolling fast. In this use case, I think scrolling performance is more important and the user's focus is on firefox versus a background tab. Once the skate animation is finished, we shrink the displayport back so the user should get their memory back to load up their VM. But yes, I don't think a giant displayport size that caches the whole page is desirable as well :)

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Adjust displayport based on system memory

Adjusts the displayport size based on available system memory. If the user has more than 4 gigs of RAM, we add 2.5 (preference) to the displayport size while skating. 

Try build (ready in about 1-2 hours from this post) - https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d8417dcb92e9

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Adjust displayport based on system memory

Comment 5

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

Comment on attachment 8666816 [details] [diff] [review]
Adjust displayport based on system memory

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

Close, but I'd like to see an updated patch with the below comments addressed. Thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +329,5 @@
>   * \li\b apz.y_stationary_size_multiplier
>   * The multiplier we apply to the displayport size if it is not skating (see
>   * documentation for the skate size multipliers above).
>   *
> + * \li\b apz.y_skate_highmem_displayport_adjust

s/y_/x_/

@@ +353,5 @@
>  /**
> + * Returns true if this is a high memory system and we can use
> + * extra memory for a larger displayport to reduce checkerboarding.
> + */
> +static bool sIsHighMemSystem = false;

make this gIsHighMemSystem

@@ +820,5 @@
>    ClearOnShutdown(&gVelocityCurveFunction);
> +
> +  const double gigabyte = 1073741824;
> +  float systemMemoryGB = (double) PR_GetPhysicalMemorySize() / gigabyte;
> +  sIsHighMemSystem = systemMemoryGB >= 4.0;

I'd prefer avoiding all the double/float/division conversions here. You can do this instead:

uint64_t sysmem = PR_GetPhysicalMemorySize();
uint64_t threshold = 1LL << 32; // 4 GB in bytes
gIsHighMemSystem = sysmem >= threshold;

@@ +2458,5 @@
>                           aCompositionSize.height + (2 * gfxPrefs::APZDangerZoneY()));
>  
> +  if (IsHighMemSystem()) {
> +    xSize += gfxPrefs::APZXSkateHighMemDisplayportAdjust();
> +    ySize += gfxPrefs::APZYSkateHighMemDisplayportAdjust();

This looks wrong. You're adding a multiplier to a pixel value. All this will do is increase the displayport by 1.5 CSS pixels which I assume is not what you want.

::: gfx/thebes/gfxPrefs.h
@@ +184,5 @@
>    DECL_GFX_PREF(Live, "apz.use_paint_duration",                APZUsePaintDuration, bool, true);
>    DECL_GFX_PREF(Live, "apz.velocity_bias",                     APZVelocityBias, float, 1.0f);
>    DECL_GFX_PREF(Live, "apz.velocity_relevance_time_ms",        APZVelocityRelevanceTime, uint32_t, 150);
>    DECL_GFX_PREF(Live, "apz.x_skate_size_multiplier",           APZXSkateSizeMultiplier, float, 1.5f);
> +  DECL_GFX_PREF(Live, "apz.x_skate_highmem_displayport_adjust",APZXSkateHighMemDisplayportAdjust, float, 1.5f);

Depending on how you end up using this pref I'd like the default value here to be either 0 or 1 (i.e. so that if the pref is not set to something else it has no effect). Also you can drop the "displayport_" from the pref names if you want to make it a bit shorter.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Adjust displayport based on system memory

Incorporated feedback from comment 5.

Comment 7

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

Comment on attachment 8666998 [details] [diff] [review]
Adjust displayport based on system memory

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

Looks good, thanks!

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa61d48eb6ae

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa61d48eb6ae

Comment 10

[Mason Chang [Inactive] [:mchang]]

I got this talos regression:

Regression: Mozilla-Inbound-Non-PGO - TP5 Scroll - WINNT 6.2 x64 (e10s) - 24.5% increase
----------------------------------------------------------------------------------------
   Previous: avg 9.865 stddev 0.148 of 12 runs up to revision d51440cc7a2fd9615d7d46bc36d67353d01b534e
   New     : avg 12.284 stddev 0.166 of 12 runs since revision aa61d48eb6ae747a5a755fb6996669720124a0d9
   Change  : +2.420 (24.5% / z=16.308)
   Graph   : http://mzl.la/1KQUnJv

Looking at https://wiki.mozilla.org/Buildbot/Talos/Tests#tp5o_scroll

"For each page, the test waits 500ms after the page load event fires, then iterates 100 scroll steps of 10px each (or until the bottom of the page is reached - whichever comes first), then reports the average frame interval."

What does average frame interval mean? How long it took to paint each frame interval?

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

there are 3 issues, tp5o_scroll, tresize and tsvgx:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=d51440cc7a2f&newProject=mozilla-inbound&newRevision=aa61d48eb6ae&e10s=1

these appear to be e10s specific issues which is good.

I will probably get a bug on file tomorrow for it :)

I am not sure what the average frame interval is, I believe :blassey wrote it up, lets ask him.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

strike that, there is 1 issue here, the tp5o_scroll on windows 8 for e10s only.

Comment 13

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/18960b8cb8e2

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Should this bug be reopened, or was it not the whole thing that got backed out?

Comment 15

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

Yup it was the whole thing.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

how long it takes to scroll each step

Comment 17

[Mason Chang [Inactive] [:mchang]]

Hi Jared,

I was unable to reproduce any black rendering spots as you reported in bug 1210411. I tried on 3 different machines, two of which had an Intel HD GPU. Can you please try this build - 

https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-88892e73104dae244aced2de1190b36b38bbbfb0/

Maybe something changed between when you reported the bug and now. Thanks!

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for the test build Mason. I tried the test build on a few long pages with text-only (https://www.gutenberg.org/files/2701/2701-h/2701-h.htm) as well as multimedia heavy pages such as CNN.com and I can't reproduce the issue.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5326ac1a435a

Comment 20

[Mason Chang [Inactive] [:mchang]]

Just an FYI, we expect some of the scrolling rendering talos tests to regress. Specifically the ones that scroll fast.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5326ac1a435a

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

thanks mason, I did a bunch of retriggers to collect more data, so far I have seen:
http://alertmanager.allizom.org:8080/alerts.html?rev=5326ac1a435a44d335752ed727fa157273b91244&showAll=1&testIndex=0&platIndex=0

that is tp5 scroll, so your predictions are correct- not sure if the magnitude of the regressions are expected.