IsLowMemory() may be incorrect on windows mobile

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
All
Windows Mobile 6 Professional
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 beta1-fixed, fennec1.0b1-wm+)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

currently we are checking dwAvailPhys / dwTotalPhys.  Over time running the browser, i am not really seeing dwAvailPhys change very much.

However, dwAvailVirtual does significantly change while using the browser.

What do you think about changing the logic to use:

dwAvailVirtual / stat.dwTotalVirtual

instead?
Posted patch patch v.1 (obsolete) — Splinter Review
this also try closing other application if we do hit a low memory situation.  I am not sure that we should do that bit here
drive by review, could you add a comment saying that 0.09 and 1024x1024 are arbitrary?  Also, what is the justification for 1024x1024?

Other than that, it looks good.
1024x1024 is also arbitrary.  It was large enough in my testing to close 3rd party applications when I started running out of OOM.
Attachment #359124 - Attachment is obsolete: true
going to rework this a bit.
GlobalMemoryStatus is very expensive, at least on CE6 -- requires a kernel VM lock and walking through a page list.  We should avoid calling it if at all possible, or at least call it only infrequently; every few seconds or so, and outside of critical performance paths.
Doug, can we just kill it off for now until we figure out a better way to do it?
Attachment #361893 - Flags: review?
Comment on attachment 361893 [details] [diff] [review]
kill off IsLowMemory for now [CHECKED IN]

it is doing the wrong thing now, and as you mentioned may have perf issues.  This is fine.

add a comment.
Attachment #361893 - Flags: review?(doug.turner) → review+
Comment on attachment 361893 [details] [diff] [review]
kill off IsLowMemory for now [CHECKED IN]

K, added comment and checked this in.
Attachment #361893 - Attachment description: kill off IsLowMemory for now → kill off IsLowMemory for now [CHECKED IN]
tracking-fennec: --- → ?
OS: Windows XP → Windows Mobile 6 Professional
Hardware: x86 → All
tracking-fennec: ? → 1.0b1-wm+
If GlobalMemoryStatus() is evil, what do we propose as a viable alternative?
Devil's Advocate: I'd like to see whether a low memory notification can actually save us in an OOM situation. Right now, it is quite hypothetical. Until we know that we can be saved from an OOM on any platform, I don't think we need to block on this.
I'd rather get session restore working, so we can recover from crashes.
what do we want to do here?

Technically, i think it can solved without calling GlobalMemoryStatus all of the time (We probably can use WM_HIBERNATE as a trigger to start calling GlobalMemoryStatus()).
crowder:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#4138

If windows sends us a WM_HIBERNATE, we should be broadcasting a low memory notification.
WM_HIBERNATE only is called when your application is in the background.  It looks like there is no other call that allows us to be notified that system memory is low.  See http://blogs.msdn.com/windowsmobile/archive/2006/08/16/702746.aspx for more information.

I think we want to revert this change for WINCE (on my devices, the call takes ~1ms).  We could consider caching the result of the system memory info, but I am not sure that is wise.
what about using _set_new_handler and _set_new_mode(1) to be notified when an allocation fails?  Or is that too late?  If its too late perhaps we can keep one jemalloc sized block of memory around and release it in the handler so we can move on with life and still issue a memory pressure event.
we are not the only application running on the device, so we want to be able to start tossing stuff before we reach an allocation failure.
also removes thread pressure stuff which didn't really do anything useful.
Attachment #400613 - Flags: review?(vladimir)
Comment on attachment 400613 [details] [diff] [review]
patch to reenable

100ms seems really aggressive; I'd make that 1000ms if not more.  If we're out of memory we may be hitting some sort of swap, so it could well be longer than 100ms just due to swapping or whatever, and we'll end up doing a lot of flushing here for probably no good reason.
Attachment #400613 - Flags: review?(vladimir) → review+
http://hg.mozilla.org/mozilla-central/rev/d2a3e5fb6a34
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #400613 - Flags: approval1.9.2?
Attachment #400613 - Flags: approval1.9.2? → approval1.9.2+
Depends on: 525323
You need to log in before you can comment on or make changes to this bug.