1414150 - Remove some unnecessary prefs in and around XPCOM

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

We have too many prefs. I see a few in and around XPCOM that can be removed easily.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove the "memory.free_dirty_pages" pref

There's no good reason why someone would want this disabled.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Remove the "memory.low_*" prefs

(dmajor, you might have an opinion about the final values for these thresholds.)

There's no good reason why these should't just be constants. The patch also
appends "MiB" to some of the C++ values to make their meaning clearer.

This patch fixes one outright bug, and one inconsistency.

The bug is due to a prefname mismatch:

- ContentPrefs.cpp and AvailableMemoryTracker.cpp use
  "memory.low_virtual_mem_threshold_mb".

- all.js uses "memory.low_virtual_memory_threshold_mb".

Which means that "memory.low_virtual_memory_threshold_mb" showed up in
about:config, but if you changed it nothing would happen because the callback
listened for changes to to "memory.low_virtual_mem_threshold_mb"!

Now for the inconsistency. The above means we actually use a value of 256 for
the virtual memory threshold, even though all.js says 128. But we *do* use a
value of 128 for the commit space threshold, because that's what all.js says
and that prefname is used correctly everywhere. The patch changes the commit
space threshold to 256 for consistency with the virtual memory threshold.

What a mess!

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Remove the "consoleservice.*" prefs

It seems fine to specify these as code constants.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Remove the "idle_queue.*" prefs

There's no good reason why these can't be code constants.

Especially given that, due to a bug, changes to the
"idle_queue.{min,long}_period" constants were not being passed onto the C++
code!

Here's why: those two prefs were specified as integers in all.js. But we used
AddFloatVarCache() to set up the reading of those prefs. libpref fakes floats
by storing them as strings and then converting them to floats when they are
read.

Which means that AddFloatVarCache() used to fail to get the value from all.js
-- because there's a type mismatch, int vs. string -- and instead use the
fallback default. That value is the same as the one in all.js, which is lucky.
But if someone changed the value in about:config to 100 (an integer), a similar
failure would have occured and the value used by the C++ code wouldn't be
updated!

Also note that idle_queue.max_timer_thread_bound did not have a value in
all.js.

What a mess!

Comment 5

[Nicholas Nethercote [inactive]]

To summarize:

- I removed one bool VarCache pref which let us disable something we'd never
  want to disable. (It probably made sense when B2G existed.)
  
- I replaced 10 numeric VarCache prefs with code constants. IMO none of these
  need to be modifiable at runtime.

- Two of the 10 numeric prefs were buggy, such that updating about:config
  wouldn't have had any effect on the C++ code.

  - One of the prefnames was spelled inconsistently.

  - One of the prefs had a type mismatch between the value specified in all.js
    and the VarCache.

Comment 6

[(Away)]

Comment on attachment 8925388 [details] [diff] [review]
Remove the "memory.low_*" prefs

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

::: xpcom/base/AvailableMemoryTracker.cpp
@@ +38,5 @@
>  #if defined(_M_IX86) && defined(XP_WIN)
>  
> +static const uint32_t kLowVirtualMemoryThresholdMiB = 256;
> +static const uint32_t kLowCommitSpaceThresholdMiB = 256;
> +static const uint32_t kLowPhysicalMemoryThresholdMiB = 0;

256 for virtual/commit seems fine.

Physical == 0 effectively disables this memory pressure event. I did some digging to figure out why it's this way, and my best guess is because bug 670967 comment 21 says "In my tests, the machine starts to page heavily when available physical memory hits 0". So maybe the tests against this variable should be a "<=" rather than "<"?

Comment 7

[(Away)]

On the other hand, bug 670967 comment 49 says "I just did some tests on my netbook, and I'm now not so convinced that the available physical memory tracking is useful." so maybe it was intentionally disabled.

Comment 8

[Nicholas Nethercote [inactive]]

Maybe I should just remove the physical memory tracking, since it's never used and apparently never has been.

Comment 9

[(Away)]

Come to think of it, we're already tracking physical memory as part of the commit-space check. The ullAvailPageFile field of MEMORYSTATUSEX is a bit misleading: it's actually the sum of available physical memory and available page file.

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8925388 [details] [diff] [review]
Remove the "memory.low_*" prefs

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

::: modules/libpref/init/all.js
@@ -5217,5 @@
> -
> -// On Windows 32-bit, don't fire a low-memory notification because of
> -// low available physical memory or low commit space more than once every
> -// low_memory_notification_interval_ms.
> -pref("memory.low_memory_notification_interval_ms", 10000);

Might be worth moving these comments to the new constants.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8925389 [details] [diff] [review]
Remove the "consoleservice.*" prefs

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

::: mobile/android/app/mobile.js
@@ -848,5 @@
>  pref("layout.accessiblecaret.extend_selection_for_phone_number", true);
>  
> -// Disable sending console to logcat on release builds.
> -#ifdef RELEASE_OR_BETA
> -pref("consoleservice.logcat", false);

I think we want to keep this one pref because it's very helpful for troubleshooting with end users running Beta, for example. Also, test harnesses may be using this pref.

Comment 12

[Bob Clary [:bc] (inactive)]

I don't think we should remove consoleservice.logcat in general. Even losing the pref on beta or release would leave a hole in our ability to test those builds. It is used implicitly on android to emit messages during unit testing. Often the only means of diagnosing failures in Unit tests on Android is to look for problems in the logcat output.

Comment 13

[Bob Clary [:bc] (inactive)]

After consulting more, defaulting to false on beta/release doesn't change the current default behavior. I still think it should be available but if it an issue I don't object.

Comment 14

[Nicholas Nethercote [inactive]]

Ok, I'll reinstate "logcat". Thanks for the feedback.

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8925385 [details] [diff] [review]
Remove the "memory.free_dirty_pages" pref

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

You may want to add a note in the commit message that the feature was originally added for b2g, so the pref was there with different values for b2g vs. !b2g, and that bug 1398033 enabled it everywhere.

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Remove the "consoleservice.{logging,enabled}" prefs

I reinstated consoleservice.logcat.

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Maybe I should just remove the physical memory tracking, since it's never
> used and apparently never has been.

Ah, but it's a "distinguished value" in memory reporting, and so used by telemetry and other things. I'll defer removal to a follow-up.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8925834 [details] [diff] [review]
Remove the "consoleservice.{logging,enabled}" prefs

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

::: xpcom/base/nsConsoleService.cpp
@@ +58,5 @@
> +static bool gLoggingLogcat =
> +#ifdef RELEASE_OR_BETA
> +  false;
> +#else
> +  true;

I think `gLoggingLogcat` should just always be initialized to `false`.

Comment 19

[Nicholas Nethercote [inactive]]

> I think `gLoggingLogcat` should just always be initialized to `false`.

Good point. I will move the RELEASE_OR_BETA true/false decision to the 3rd argument  of the AddBoolVarCache() call, to match mobile.js. (It's horrible how we currently duplicate defaults in the *.js initialization files and the Add*VarCache() calls; that's something I want to fix eventually.)

Comment 20

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b84d87e9fa100abe4f3aa073d2e169d3392d9b4c
Bug 1414150 - Remove the "memory.free_dirty_pages" pref. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/99a32740c0588f68bc391a3ae24fe478782134ac
Bug 1414150 - Remove the "memory.low_*" prefs. r=erahm,dmajor.

https://hg.mozilla.org/integration/mozilla-inbound/rev/242b1157a36a8fbe769179a630c8457562d9568f
Bug 1414150 - Remove the "consoleservice.{logging,enabled}" prefs. r=jchen.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6063357ae629ffb82e2087ac58431c07548a82
Bug 1414150 - Remove the "idle_queue.*" prefs. r=farre.

Comment 21

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8925834 [details] [diff] [review] [diff] [review]
Remove the "consoleservice.{logging,enabled}" prefs

Having static gLoggingBuffered = true without anyway to modify the value makes if (!gLoggingBuffered) block effectively dead code and if (gLoggingBuffered) {} block unconditionally true. Why not clean them up further?

Comment 22

[Nicholas Nethercote [inactive]]

> Having static gLoggingBuffered = true without anyway to modify the value
> makes if (!gLoggingBuffered) block effectively dead code and if
> (gLoggingBuffered) {} block unconditionally true. Why not clean them up
> further?

I deliberately left them like that so that if somebody does want to change those options, they can do it by changing a single line of code. But reasonable people could disagree about whether that was the right choice...

Comment 23

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b84d87e9fa10
https://hg.mozilla.org/mozilla-central/rev/99a32740c058
https://hg.mozilla.org/mozilla-central/rev/242b1157a36a
https://hg.mozilla.org/mozilla-central/rev/dd6063357ae6

Comment 24

[Graham Perrin]

(In reply to Nicholas Nethercote [:njn] (on PTO until Jan 29, 2018) from comment #1)

> … "memory.free_dirty_pages" pref
> 
> There's no good reason why someone would want this disabled.

Please, is the same true for 56.x? 

I'm interested from a Waterfox 56.x perspective. 

TIA

Comment 25

[Nicholas Nethercote [inactive]]

> > … "memory.free_dirty_pages" pref
> > 
> > There's no good reason why someone would want this disabled.
> 
> Please, is the same true for 56.x? 
> 
> I'm interested from a Waterfox 56.x perspective. 

I believe it is true. Disabling that pref will increase memory usage, while being extremely unlikely to provide any other benefits.

Comment 26

[Graham Perrin]

Thank you! 

memory.free_dirty_pages in Waterfox 56.x · Issue #423 · MrAlex94/Waterfox
<https://github.com/MrAlex94/Waterfox/issues/423>