Closed Bug 828887 Opened 7 years ago Closed 7 years ago

Add some tunable GC parameters to workers

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
b2g18 + fixed

People

(Reporter: jlal, Assigned: gwagner)

References

Details

(Whiteboard: [MemShrink])

Attachments

(2 files, 1 obsolete file)

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.
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.
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: 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?
(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?
Attached patch patch (obsolete) — Splinter Review
Attached patch better patchSplinter Review
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+
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.
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/mozilla-central/rev/b5bf34b9a55e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 841041
Depends on: 841962
You need to log in before you can comment on or make changes to this bug.