475595 - IsLowMemory() may be incorrect on windows mobile

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

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?

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

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

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 3

[Doug Turner (:dougt)]

1024x1024 is also arbitrary.  It was large enough in my testing to close 3rd party applications when I started running out of OOM.

Comment 4

[Doug Turner (:dougt)]

going to rework this a bit.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: kill off IsLowMemory for now [CHECKED IN]

Doug, can we just kill it off for now until we figure out a better way to do it?

Comment 7

[Doug Turner (:dougt)]

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.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 361893 [details] [diff] [review]
kill off IsLowMemory for now [CHECKED IN]

K, added comment and checked this in.

Comment 9

[Brian Crowder]

If GlobalMemoryStatus() is evil, what do we propose as a viable alternative?

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

I'd rather get session restore working, so we can recover from crashes.

Comment 12

[Doug Turner (:dougt)]

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

Comment 13

[Doug Turner (:dougt)]

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.

Comment 14

[Doug Turner (:dougt)]

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.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 16

[Doug Turner (:dougt)]

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.

Comment 17

[Doug Turner (:dougt)]

Attachment: patch to reenable

also removes thread pressure stuff which didn't really do anything useful.

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 19

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/d2a3e5fb6a34

Comment 20

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/4313fb6cb7a3  <-- cleans up a comment.

Comment 21

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9e6cd2038958