Closed
Bug 1451002
Opened 6 years ago
Closed 6 years ago
Ensure ongoing memory pressure notifications are sent and dealt with correctly
Categories
(Core :: General, enhancement)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: gsvelto, Assigned: gsvelto)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file, 1 obsolete file)
When memory is low we can send memory-pressure notifications by calling NS_DispatchEventualMemoryPressure(). The first time this should be called with the MemPressure_New parameter to indicate that we just entered a low-memory scenario. If the memory pressure does not recede by the time we check again the call should use MemPressure_Ongoing [1] instead. This is crucial because sending another MemPressure_New event might cause us to run CPU-intensive operations (such as GC/CC) that are unlikely to free additional memory. In a worst-case scenario when might actually be sending a notification before the previous one has finished executing (see bug 1306128 and bug 1307018). There are two issues to be fixed to address this: the first one is making the AvailableMemoryTracker detect ongoing memory pressure scenarios and send MemPressure_Ongoing events. Currently it only sends MemPressure_New ones [2]. The second issue is that currently only the GC/CC handles ongoing memory pressure events properly [3]. We should check if there are other listeners that might react poorly if ongoing events are treated as new ones. As a rule of thumb, listeners that do something very CPU intensive to free memory should not react to ongoing memory-pressure events. [1] https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/xpcom/threads/nsMemoryPressure.h#43 [2] https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/xpcom/base/AvailableMemoryTracker.cpp#112 [3] https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/dom/base/nsJSEnvironment.cpp#333
Assignee | ||
Updated•6 years ago
|
Whiteboard: [MemShrink]
Updated•6 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•6 years ago
|
||
This patch detects if we're already in a low-memory state and send ongoing memory-pressure notifications instead of new ones. I've tried to keep the logical changes to a minimum but I also tried to make the existing code a little more readable. Additionally I've changed the statistics counters we use in the detection logic to use relaxed atomics. No point in emitting full memory barriers for those type of variables. I've been itching to actually implement a simple spinlock to protect the logic but resisted as it would go well outside the scope of this patch.
Attachment #8969047 -
Flags: review?(n.nethercote)
Comment 2•6 years ago
|
||
Comment on attachment 8969047 [details] [diff] [review] [PATCH] Send ongoing memory pressure notifications when a low-memory condition persists for a long time Review of attachment 8969047 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/AvailableMemoryTracker.cpp @@ +72,1 @@ > volatile PRIntervalTime sLastLowMemoryNotificationTime; Ugh, why are these volatile? Presumably they should be atomic. (Pre-existing problem, no need to change.)
Attachment #8969047 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #2) > Ugh, why are these volatile? Presumably they should be atomic. (Pre-existing > problem, no need to change.) Yeah, pretty ugly but I didn't want to touch them in this patch. I'll file another bug to fix the races in this code. It's just a matter of using atomics to protect this section with a simple busy-polled spin-lock and make those variables ReleaseAcquire so that their updates are seen correctly by another thread when entering the protected section.
Assignee | ||
Comment 4•6 years ago
|
||
Also, I'm still testing this on my machine to ensure I'm not regressing anything. I'm currently injecting artificial low-memory conditions to simulate the real behavior. I'm musing about turning this into a thing that could be used in an xpcshell test.
Assignee | ||
Comment 5•6 years ago
|
||
Still testing this, I've noticed another bug in the code that I hadn't spotted before: if kLowPhysicalMemoryThreshold is 0, then we won't check the commmit-space threshold. So if both kLowPhysicalMemoryThreshold and kLowVirtualMemoryThreshold are set 0 we'll never check for low memory, even if kLowCommitSpaceThreshold is non zero.
Assignee | ||
Comment 6•6 years ago
|
||
Updated patch after testing this extensively. There was a pretty bad mistake in the first iteration that needed fixing and I also cleaned up the comments to reflect the new behavior. Carrying over the r+ flag.
Attachment #8969047 -
Attachment is obsolete: true
Attachment #8969648 -
Flags: review+
Pushed by gsvelto@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9dcde577687c Send ongoing memory pressure notifications when a low-memory condition persists for a long time; r=njn
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9dcde577687c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 9•6 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #0) > The second issue is that currently only the GC/CC handles ongoing memory > pressure events properly [3]. We should check if there are other listeners > that might react poorly if ongoing events are treated as new ones. As a rule > of thumb, listeners that do something very CPU intensive to free memory > should not react to ongoing memory-pressure events. So if my reading is correct, it seems like this patch will reduce the number of GCs on desktop and have no effect on Android. On desktop, this would be handled by https://searchfox.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#321 which does a GC on MemPressure_New but not MemPressure_Ongoing. On Android, javascript_options_gc_on_memory_pressure is false and instead we use https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/MemoryObserver.js#15 which only checks for "heap-minimize". Is that correct? If so, should Android behave differently for new vs ongoing notifications as well?
Flags: needinfo?(gsvelto)
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #9) > So if my reading is correct, it seems like this patch will reduce the number > of GCs on desktop and have no effect on Android. Yes, exactly. > On desktop, this would be handled by > https://searchfox.org/mozilla-central/source/dom/base/nsJSEnvironment. > cpp#321 which does a GC on MemPressure_New but not MemPressure_Ongoing. > > On Android, javascript_options_gc_on_memory_pressure is false and instead we > use > https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/ > MemoryObserver.js#15 which only checks for "heap-minimize". > > Is that correct? If so, should Android behave differently for new vs ongoing > notifications as well? Android is doing the right thing, the issue with desktop for Windows was that ongoing memory pressure notifications were not being sent hence causing excessive GC cycles if the low memory scenario didn't solve itself after the first notification.
Flags: needinfo?(gsvelto)
Comment 11•6 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #10) > > On Android, javascript_options_gc_on_memory_pressure is false and instead we > > use > > https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/ > > MemoryObserver.js#15 which only checks for "heap-minimize". > > > > Is that correct? If so, should Android behave differently for new vs ongoing > > notifications as well? > > Android is doing the right thing, the issue with desktop for Windows was > that ongoing memory pressure notifications were not being sent hence causing > excessive GC cycles if the low memory scenario didn't solve itself after the > first notification. Hm. I guess I'm still curious -- why is it ok to repeat GCs on Android but not on desktop? Is the idea that running out of memory is a more serious threat on Android, but lagging everything to death is more serious on desktop? (Maybe because Android doesn't have any swap or something.)
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #11) > Hm. I guess I'm still curious -- why is it ok to repeat GCs on Android but > not on desktop? Is the idea that running out of memory is a more serious > threat on Android, but lagging everything to death is more serious on > desktop? (Maybe because Android doesn't have any swap or something.) My bad, I misread the code. The same logic should apply to Android too, it shouldn't run a garbage collection if the memory-pressure notification is an ongoing one. In fact on Android this would be even worse because a GC cycle is rather CPU intensive so on weaker CPUs we'd be causing a lot of lag for very little gain. Let's file a bug about that.
You need to log in
before you can comment on or make changes to this bug.
Description
•