Closed Bug 1301667 Opened 3 years ago Closed 3 years ago

Increase the threshold of low memory notifications on Windows

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

()

Details

Attachments

(1 file, 1 obsolete file)

As discussed on dev-platform we often run into OOM crashes when we still have more virtual address space available than the limit which triggers a memory pressure event. This means that the available memory tracker will often not send a memory pressure event when memory gets low in turn making this mechanism less effective in preventing OOMs.

The current threshold is set at 128MiB; I'd like to increase it to 256MiB which seems like an acceptable level for machines with 1GB+ of physical memory.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Here's a tentative patch to address this, note that this only affects 32-bit Windows:

- The virtual and physical memory thresholds that trigger a low memory condition are increased to 256 MiB

- I've modified the comments to make it clear that this applies only to 32-bit Windows

- Code that wouldn't run on 64-bit was kind of, partially disabled at compile time, I've ifdef'd it out entirely since it's not used there

- I took the chance to remove the custom logging and debug functions that were used during development; this code has been working for ages so I doubt we need them and they just make the code more confusing
Attachment #8789776 - Flags: feedback?(n.nethercote)
Attachment #8789776 - Flags: feedback?(benjamin)
Comment on attachment 8789776 [details] [diff] [review]
[PATCH] Increase the low memory threshold under Windows and clean up the available memory tracker

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

Apologies for the slow response time.

Increasing the virtual memory threshold definitely seems like a good idea. I've seen plenty of OOM crashes where the available virtual memory was 200 MiB or so. As for increasing the commit space threshold -- do we have evidence that this will make a difference? I don't object to the change, but I'd be interested to know one way or the other.

The other changes look fine.

::: xpcom/base/AvailableMemoryTracker.cpp
@@ +260,3 @@
>  
>      MOZ_COLLECT_REPORT(
>        "low-commit-space-events", KIND_OTHER, UNITS_COUNT_CUMULATIVE,

These events are given a different path to the other two, so we end up with this output structure in about:memory:

> low-memory-events
>   - physical
>   - virtual
>
> ...
>
> low-commit-space-events

I don't see a good reason for this separation, so I suggest changing this path to "low-memory-events/commit-space" so all three events end up reported in the same tree. (This is a pre-existing thing, but might as well fix it while you're in here.)
Attachment #8789776 - Flags: feedback?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #2)
> Increasing the virtual memory threshold definitely seems like a good idea.
> I've seen plenty of OOM crashes where the available virtual memory was 200
> MiB or so. As for increasing the commit space threshold -- do we have
> evidence that this will make a difference? I don't object to the change, but
> I'd be interested to know one way or the other.

I don't have any solid evidence for that but 128MiB of commit space left is very little memory available. Hitting such a low threshold without hitting the virtual space limit means that the underlying machine has very little physical memory available and I think it would be useful to send memory pressure events in that case.

> These events are given a different path to the other two, so we end up with
> this output structure in about:memory:
> 
> > low-memory-events
> >   - physical
> >   - virtual
> >
> > ...
> >
> > low-commit-space-events
> 
> I don't see a good reason for this separation, so I suggest changing this
> path to "low-memory-events/commit-space" so all three events end up reported
> in the same tree. (This is a pre-existing thing, but might as well fix it
> while you're in here.)

I'll do that.
Updated patch with the commit-space events telemetry path changed to low-memory-events/commit-space
Attachment #8789776 - Attachment is obsolete: true
Attachment #8789776 - Flags: feedback?(benjamin)
Attachment #8790689 - Flags: review?(n.nethercote)
Attachment #8790689 - Flags: review?(n.nethercote) → review+
Thanks Nicholas, here's the green try-run (save for the usual orange I had to re-trigger):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c2e9358c925

Landing soon.
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8bbd5046a4
Increase the low memory threshold under Windows and clean up the available memory tracker r=njn
https://hg.mozilla.org/mozilla-central/rev/1b8bbd5046a4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.