Remove some unnecessary prefs in and around XPCOM

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

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

Comment 1

2 years ago
There's no good reason why someone would want this disabled.
Attachment #8925385 - Flags: review?(mh+mozilla)
Assignee

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee

Comment 2

2 years ago
(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!
Attachment #8925388 - Flags: review?(erahm)
Attachment #8925388 - Flags: feedback?(dmajor)
Assignee

Comment 3

2 years ago
It seems fine to specify these as code constants.
Attachment #8925389 - Flags: review?(nchen)
Assignee

Comment 4

2 years ago
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!
Attachment #8925390 - Flags: review?(afarre)
Assignee

Comment 5

2 years ago
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.
Attachment #8925390 - Flags: review?(afarre) → review+
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 "<"?
Attachment #8925388 - Flags: feedback?(dmajor) → feedback+
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.
Assignee

Comment 8

2 years ago
Maybe I should just remove the physical memory tracking, since it's never used and apparently never has been.
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 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.
Attachment #8925388 - Flags: review?(erahm) → review+
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.
Attachment #8925389 - Flags: review?(nchen) → feedback+

Comment 12

2 years ago
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

2 years ago
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.
Assignee

Comment 14

2 years ago
Ok, I'll reinstate "logcat". Thanks for the feedback.
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.
Attachment #8925385 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 16

2 years ago
I reinstated consoleservice.logcat.
Attachment #8925834 - Flags: review?(nchen)
Assignee

Updated

2 years ago
Attachment #8925389 - Attachment is obsolete: true
Assignee

Comment 17

2 years ago
(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 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`.
Attachment #8925834 - Flags: review?(nchen) → review+
Assignee

Comment 19

2 years ago
> 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 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?
Assignee

Comment 22

2 years ago
> 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...
Depends on: 1417185

Comment 24

2 years ago
(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
Assignee

Comment 25

Last year
> > … "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

Last year
Thank you! 

memory.free_dirty_pages in Waterfox 56.x · Issue #423 · MrAlex94/Waterfox
<https://github.com/MrAlex94/Waterfox/issues/423>
See Also: → 1451013
You need to log in before you can comment on or make changes to this bug.