Closed Bug 1450314 Opened 2 years ago Closed 2 years ago

Unused memory-pressure handling bits

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: JanH, Assigned: JanH)

Details

Attachments

(1 file)

Looking at https://dxr.mozilla.org/mozilla-central/rev/dcd10220d55aea46db212314c46d25a96a7be243/xpcom/base/nsIMemory.idl,
I've noticed some things being described that still have remnants in our codebase, but aren't actively used any more:
- low-memory-ongoing: Still checked for in nsJSEnvironment.cpp [1], but currently no one is actually calling NS_Dispatch(Eventual)MemoryPressure with MemPressure_Ongoing. As far as I can tell, only B2G ever did.
- alloc-failure: Completely unused outside of that IDL file and looking through the ESR repositories, last seen in mozilla-esr10
- lowering-priority: Still checked for in the PuppetWidget [2], but the code that was actually sending this was added in bug 1181518 and removed again in bug 1234176

For the latter two I'm fairly certain that they can go, but for low-memory-ongoing I'm a bit unsure whether we might want to revive it or remove it for good.

[1] https://dxr.mozilla.org/mozilla-central/rev/dcd10220d55aea46db212314c46d25a96a7be243/dom/base/nsJSEnvironment.cpp#334
[2] https://dxr.mozilla.org/mozilla-central/rev/dcd10220d55aea46db212314c46d25a96a7be243/widget/PuppetWidget.cpp#1153
Nathan, do you have any opinion on the above, or know somebody who does? I might be touching this code for bug 1335148 and not having to worry about low-memory-ongoing might make things slightly nicer, although it's not a huge issue.
Flags: needinfo?(nfroyd)
My inclination is to remove them all, but njn is the nsIMemory.idl expert, so he gets the last word.
Flags: needinfo?(nfroyd) → needinfo?(n.nethercote)
gsvelto is talking about reviving some of the memory pressure stuff, so I'd say keep the low-memory one. The other two can probably go.
I agree about alloc-failure and lowering-priority, neither of those should serve a purpose anymore. The fact that we're not using low-memory-ongoing however is a genuine bug and it has come back to bite us as soon as we actually started getting memory-pressure notifications. I've filed bug 1450993 to gather all the issues that need to be addressed to get this machinery up to snuff and bug 1451002 is specifically about ongoing memory pressure notifications. I plan on working on those bugs soon because our OOM crash rate has raised quite significantly with 58.
I defer to gsvelto :)
Flags: needinfo?(n.nethercote)
Comment on attachment 8973508 [details]
Bug 1450314 - Remove unused parameters for memory-pressure notifications.

https://reviewboard.mozilla.org/r/241870/#review248110

::: widget/PuppetWidget.cpp
(Diff revision 1)
>    if (!mWidget) {
>      return NS_OK;
>    }
>  
> -  if (strcmp("memory-pressure", aTopic) == 0 &&
> +  if (strcmp("memory-pressure", aTopic) == 0) {
> -      !NS_LITERAL_STRING("lowering-priority").Equals(aData)) {

This is a change in behaviour. Currently, this code is never executed because aData never matches "lowering-priority".

gsvelto, what do you think should happen here?
Attachment #8973508 - Flags: review?(n.nethercote) → review?(gsvelto)
Comment on attachment 8973508 [details]
Bug 1450314 - Remove unused parameters for memory-pressure notifications.

https://reviewboard.mozilla.org/r/241870/#review248172

This looks good to me but I think we should special-case this listener to not release resources if we're in an ongoing memory-pressure scenario, see below.

::: widget/PuppetWidget.cpp
(Diff revision 1)
>    if (!mWidget) {
>      return NS_OK;
>    }
>  
> -  if (strcmp("memory-pressure", aTopic) == 0 &&
> +  if (strcmp("memory-pressure", aTopic) == 0) {
> -      !NS_LITERAL_STRING("lowering-priority").Equals(aData)) {

This code is tricky. What it originally did before bug 1154231 this code caller ClearCachedResources() as soon as the widget became invisible. Back in the FxOS days this became a problem: when an application was sent to the background and then restored it would cause visible glitches before the layers were rebuilt. So this was changed to invoke ClearCachedResources() only on memory pressure, but the issue popped up again because we started sending memory pressure events to any process that was sent in the background, hence the introduction of lowering-priority. In short, it was a kludge we had to deploy as we were desperately trying to keep processes alive in a memory-starved environment but we didn't want visual glitches when they became visible again.

That being said I think this change is safe. memory-pressure events are rare enough that we should honor them and try saving as much memory as possible. However it might be worth special-casing this to not respond to events with a low-memory-ongoing payload. Clearing these resources a second time when memory pressure hasn't receeded might have an annoying visible impact while saving little memory.
Attachment #8973508 - Flags: review?(gsvelto) → review+
Assignee: nobody → jh+bugzilla
Comment on attachment 8973508 [details]
Bug 1450314 - Remove unused parameters for memory-pressure notifications.

https://reviewboard.mozilla.org/r/241870/#review248110

> This is a change in behaviour. Currently, this code is never executed because aData never matches "lowering-priority".
> 
> gsvelto, what do you think should happen here?

Isn't it the other way round - the code below is always executed because nobody is setting aData to "lowering-priority" any more?

But in any case I've followed gsevlto's suggestions to exclude the case of "low-memory-ongoing" instead.
(In reply to Jan Henning [:JanH] from comment #10)
> Isn't it the other way round - the code below is always executed because
> nobody is setting aData to "lowering-priority" any more?

Actually yes :)
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/e642f111b291
Remove unused parameters for memory-pressure notifications. r=gsvelto
https://hg.mozilla.org/mozilla-central/rev/e642f111b291
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.