IsLowMemory() may be incorrect on windows mobile

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
All
Windows Mobile 6 Professional
Points:
---

Firefox Tracking Flags

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

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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?
(Assignee)

Comment 1

9 years ago
Created attachment 359124 [details] [diff] [review]
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
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.
(Assignee)

Comment 3

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

Updated

9 years ago
Attachment #359124 - Attachment is obsolete: true
(Assignee)

Comment 4

9 years ago
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.
Created attachment 361893 [details] [diff] [review]
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?
Attachment #361893 - Flags: review?
Attachment #361893 - Flags: review? → review?(doug.turner)
(Assignee)

Comment 7

9 years ago
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]
(Assignee)

Updated

9 years ago
tracking-fennec: --- → ?
OS: Windows XP → Windows Mobile 6 Professional
Hardware: x86 → All
tracking-fennec: ? → 1.0b1-wm+

Comment 9

9 years ago
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.
(Assignee)

Comment 12

9 years ago
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()).
(Assignee)

Comment 13

8 years ago
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.
(Assignee)

Comment 14

8 years ago
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.
(Assignee)

Comment 16

8 years ago
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.
(Assignee)

Comment 17

8 years ago
Created attachment 400613 [details] [diff] [review]
patch to reenable

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

Updated

8 years ago
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+
(Assignee)

Comment 19

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d2a3e5fb6a34
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #400613 - Flags: approval1.9.2?
(Assignee)

Comment 20

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

Updated

8 years ago
Attachment #400613 - Flags: approval1.9.2? → approval1.9.2+
(Assignee)

Comment 21

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9e6cd2038958
status1.9.2: --- → beta1-fixed
Depends on: 525323
You need to log in before you can comment on or make changes to this bug.