1301667 - Increase the threshold of low memory notifications on Windows

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Gabriele Svelto [:gsvelto]]

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.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Increase the low memory threshold under Windows and clean up the available memory tracker

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

Comment 2

[Nicholas Nethercote [inactive]]

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.)

Comment 3

[Gabriele Svelto [:gsvelto]]

(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.

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Increase the low memory threshold under Windows and clean up the available memory tracker

Updated patch with the commit-space events telemetry path changed to low-memory-events/commit-space

Comment 5

[Gabriele Svelto [:gsvelto]]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b8bbd5046a4