Closed Bug 1414150 Opened 7 years ago Closed 7 years ago

Remove some unnecessary prefs in and around XPCOM

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Remove the "memory.free_dirty_pages" pref
3.89 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Remove the "memory.low_*" prefs
8.79 KB, patch
erahm
: review+
away
: feedback+
Details | Diff | Splinter Review
Remove the "idle_queue.*" prefs
7.39 KB, patch
farre
: review+
Details | Diff | Splinter Review
Remove the "consoleservice.{logging,enabled}" prefs
4.59 KB, patch
jchen
: review+
Details | Diff | Splinter Review 
We have too many prefs. I see a few in and around XPCOM that can be removed easily.
There's no good reason why someone would want this disabled.
Attachment #8925385 - Flags: review?(mh+mozilla)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(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)
It seems fine to specify these as code constants.
Attachment #8925389 - Flags: review?(nchen)
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)
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.
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+
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.
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.
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+
I reinstated consoleservice.logcat.
Attachment #8925834 - Flags: review?(nchen)
Attachment #8925389 - Attachment is obsolete: true
(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+
> 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?
> 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
(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
> > … "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.
Thank you! 

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

Attachment

General

Creator:
Created:
Updated:
Size: