Add some tunable GC parameters to workers

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: lightsofapollo, Assigned: gwagner)

Tracking

(Blocks: 1 bug)

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-, firefox19 wontfix, firefox20 wontfix, b2g18+ fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 1 obsolete attachment)

1.32 MB, text/plain
Details
16.94 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Justin Lebar (not reading bugmail)
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 700261 [details]
Partially through a sync operation before a OOM crash in FFOS.

Accounts for some of the memory issues in the gaia calendar app which essentially is constantly streaming data (via xhr + moz-chunked-text) and parsing xml/ical data.

In my case the worker is always busy with some CPU/memory intensive task until we kill it so it is never idle.
Whiteboard: [MemShrink]
Does your worker return to the event loop regularly, or does it get a whole bunch of data at the beginning and then process it all without ever returning to the event loop?

We need to make both cases work, but I specifically want to make sure we fix your case.
Blocks: 797189
Assignee: general → nobody
Component: JavaScript Engine → DOM: Workers
Here's the bad part of attachment 700261 [details]:

> 61.04 MB (100.0%) -- explicit
> ├──31.24 MB (51.18%) -- workers/workers(gaiamobile.org)/worker(/caldav_worker.js?time=1357810196506, 43fc4800)
> │  ├──22.57 MB (36.97%) -- gc-heap
> │  │  ├──20.19 MB (33.08%) ── unused-arenas
> │  │  ├───2.00 MB (03.28%) ── unused-chunks

22.5mb of decommitable JS memory.  Ouch.
(Assignee)

Comment 3

5 years ago
We currently have a 30MB allocation allowance before we start GCs in workers. We have many workers so we should definitely change this.
blocking-basecamp: --- → ?
(Assignee)

Updated

5 years ago
Assignee: nobody → anygregor
Its hard to determine bb+ with the information in this bug.  Can you help us better understand the user impact of this bug?  Just OOM crashing for Calendar app?  How frequent?
(Assignee)

Comment 5

5 years ago
(In reply to Lucas Adamski from comment #4)
> Its hard to determine bb+ with the information in this bug.  Can you help us
> better understand the user impact of this bug?  Just OOM crashing for
> Calendar app?  How frequent?

I am still investigating. The problem is that JS workers get the default allocation threshold from desktop firefox. This is optimized for not performing GC during startup and is set to 30MB.

We have many workers on B2G and depending on how the worker is used it might always allocate 30MB. Note that the threshold is per worker and not for all the workers combined.

I tested a patch with James that reduces the GC threshold to 3MB for workers. The worker in question here reduced the footprint from 30MB to around 3-5 MB.

I haven't looked at other workers we are using but if we do some memory intensive computation we are in a much better situation with this patch.
(In reply to Lucas Adamski from comment #4)
> Its hard to determine bb+ with the information in this bug.  Can you help us
> better understand the user impact of this bug?  Just OOM crashing for
> Calendar app?  How frequent?

It's also going to cause excessive memory usage in any app which uses workers to do complicated but finite tasks (e.g. syncing stuff).  So I expect we'd see high memory usage in the mail app as well as a result of this bug.

The user impact of high memory usage is crashes in the worst case, and otherwise reduced multitasking (e.g. your music app in the background crashes when you sync your e-mail).
Sounds like a very simple fix tho with a risk for regressions.  We'd definitely take a fix but don't think we'd stop ship of the phone for this.
blocking-basecamp: ? → -
tracking-b2g18: --- → +
Do any workers in the chrome process allocate memory dynamically?
(Assignee)

Comment 9

5 years ago
Created attachment 700376 [details] [diff] [review]
patch
(Assignee)

Comment 10

5 years ago
Created attachment 700667 [details] [diff] [review]
better patch
Attachment #700376 - Attachment is obsolete: true
Comment on attachment 700667 [details] [diff] [review]
better patch

Review of attachment 700667 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!

::: dom/workers/RuntimeService.cpp
@@ +1384,2 @@
>  {
> +  BROADCAST_ALL_WORKERS(UpdateJSWorkerMemoryParameter, key, GetDefaultJSWorkerMemoryParameter(key));

Nit: 80 chars wrapping

::: dom/workers/RuntimeService.h
@@ +168,5 @@
> +      case JSGC_ALLOCATION_THRESHOLD:
> +        return sDefaultJSAllocationThreshold;
> +      case JSGC_MAX_BYTES:
> +        return sDefaultJSRuntimeHeapSize;
> +      default:

MOZ_NOT_REACHED again.

::: dom/workers/WorkerPrivate.cpp
@@ +1839,5 @@
>      AssertIsOnMainThread();
>  
>      mJSContextOptions = RuntimeService::GetDefaultJSContextOptions();
> +    mJSRuntimeHeapSize = RuntimeService::GetDefaultJSWorkerMemoryParameter(JSGC_MAX_BYTES);
> +    mJSWorkerAllocationThreshold = RuntimeService::GetDefaultJSWorkerMemoryParameter(JSGC_ALLOCATION_THRESHOLD);

Nit: Wrap this so it fits under 80 chars.

@@ +2218,5 @@
> +    case JSGC_MAX_BYTES:
> +      mJSRuntimeHeapSize = aValue;
> +      break;
> +    default:
> +      break;

I'd just s/break/MOZ_NOT_REACHED/ here.

@@ +2222,5 @@
> +      break;
> +  }
> +
> +  nsRefPtr<UpdateJSWorkerMemoryParameterRunnable> runnable =
> +    new UpdateJSWorkerMemoryParameterRunnable(ParentAsWorkerPrivate(), aKey, aValue);

Nit: 80 chars?
Attachment #700667 - Flags: review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5bf34b9a55e
(Assignee)

Comment 13

5 years ago
Comment on attachment 700667 [details] [diff] [review]
better patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: OOM situations and crashes when using workers (email, calendar, ...)
Testing completed: yes
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none
Attachment #700667 - Flags: approval-mozilla-b2g18?
This patch doesn't fix the bug as described, right?  We do more incremental GCs with this patch (which is good), but this patch doesn't make us do more shrinking GCs, right?

Perhaps we should morph this bug and file a new bug for the symptoms originally described here.
Blocks: 829482
Summary: "unused-arenas" is not freed unless worker is idle for 5 seconds → Add some tunable GC parameters to workers
Comment on attachment 700667 [details] [diff] [review]
better patch

Considering how much we use workers, I think the reward here outweighs the risk.  All we're doing here is setting JS runtime params for worker runtimes that we already set for the main JS runtime.

Nobody has told me that my memory-fix approval powers have run out, so a=me, I guess!
Attachment #700667 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1fa736d4b087
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
https://hg.mozilla.org/mozilla-central/rev/b5bf34b9a55e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/b5bf34b9a55e
https://hg.mozilla.org/mozilla-central/rev/b5bf34b9a55e
(Assignee)

Updated

5 years ago
Depends on: 841041
Depends on: 841962
You need to log in before you can comment on or make changes to this bug.