Closed Bug 1208636 Opened 7 years ago Closed 7 years ago

Adjust displayport size based on available system memory

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed
firefox45 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
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.
(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 :)
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/mchang@mozilla.com-d8417dcb92e9
Attachment #8666806 - Flags: review?(bugmail.mozilla)
Attachment #8666806 - Flags: review?(bugmail.mozilla)
Attachment #8666816 - Flags: review?(bugmail.mozilla)
Attachment #8666806 - Attachment is obsolete: true
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.
Attachment #8666816 - Flags: review?(bugmail.mozilla) → review-
Incorporated feedback from comment 5.
Attachment #8666816 - Attachment is obsolete: true
Attachment #8666998 - Flags: review?(bugmail.mozilla)
Comment on attachment 8666998 [details] [diff] [review]
Adjust displayport based on system memory

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

Looks good, thanks!
Attachment #8666998 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/aa61d48eb6ae
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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?
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher) → needinfo?(blassey.bugs)
strike that, there is 1 issue here, the tp5o_scroll on windows 8 for e10s only.
Should this bug be reopened, or was it not the whole thing that got backed out?
Yup it was the whole thing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
how long it takes to scroll each step
Flags: needinfo?(blassey.bugs)
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/mchang@mozilla.com-88892e73104dae244aced2de1190b36b38bbbfb0/

Maybe something changed between when you reported the bug and now. Thanks!
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Just an FYI, we expect some of the scrolling rendering talos tests to regress. Specifically the ones that scroll fast.
Flags: needinfo?(jmaher)
https://hg.mozilla.org/mozilla-central/rev/5326ac1a435a
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(jmaher)
You need to log in before you can comment on or make changes to this bug.