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.

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.

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.

We currently have a 30MB allocation allowance before we start GCs in workers. We have many workers so we should definitely change this.

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?

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

Do any workers in the chrome process allocate memory dynamically?

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?

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

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.

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!