829482 - Should do shrinking GCs more often for workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

+++ This bug was initially created as a clone of Bug #828887 +++

We should do shrinking GC more often for workers.

Comment 1

[Nicholas Nethercote [inactive]]

gwagner, would you be able to look at this?  jlebar says there are some easy wins to be had here.

Comment 2

[Nicholas Nethercote [inactive]]

I'm confused:  bug 828887 landed and the message for the patch was '"unused-arenas" is not freed unless worker is idle for 5 seconds.'.  So has this been done or not?  Should the title of this bug be changed?

Comment 3

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> I'm confused:  bug 828887 landed and the message for the patch was
> '"unused-arenas" is not freed unless worker is idle for 5 seconds.'.  So has
> this been done or not?  Should the title of this bug be changed?

The commit message was incorrect.  That change made us GC workers more eagerly, which solved the acute problem of the calendar app having tens of mb of decommittable memory.

Comment 4

[Gregor Wagner [:gwagner]]

bent: i wonder if gwagner wants that assigned to him

yeah my answer is no :)

Comment 6

[Justin Lebar (not reading bugmail)]

This is wasting upwards of 3mb in the b2g main process (bug 864436), which is quite bad.

Comment 7

[Nicholas Nethercote [inactive]]

In the profile jlebar posted to dev-platform three of the main process workers had 4.98 MiB of unused-arenas between them.  (The fourth -- one of the wifi workers -- had none, interestingly, and was correspondingly much smaller than the other three.)

Comment 8

[Andrew McCreight (out of office until 8/21) [:mccr8]]

In the browser main process we have a whole separate timer devoted to triggering shrinking GCs, so maybe that's why we don't see this there.  IIRC, Terrence added that.

Comment 9

[Terrence Cole [:terrence]]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> In the browser main process we have a whole separate timer devoted to
> triggering shrinking GCs, so maybe that's why we don't see this there. 
> IIRC, Terrence added that.

We had shrinking GC's before -- mostly for flushing caches -- I just expanded on it a bit. I have no idea how the browser or B2G triggers these as compared to normal GCs.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch for mozilla-b2g18

This does several things we talked about:

* free committed arena threshold turned down to 1mb
* reduced allocation threashold to 1mb
* worker periodic timer goes to 5s, calls maybegc
* use other gc pref values that we weren't before

On trunk I'd like to do basically this but allow worker-specific overrides. That can wait.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 743961 [details] [diff] [review]
Patch for mozilla-b2g18

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

::: dom/workers/WorkerPrivate.cpp
@@ +69,3 @@
>  
>  // GC will run five seconds after the last event is processed.
> +#define IDLE_GC_TIMER_DELAY_MS 2000

Update the comments.

Comment 12

[Gregor Wagner [:gwagner]]

Comment on attachment 743961 [details] [diff] [review]
Patch for mozilla-b2g18

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

::: dom/workers/WorkerPrivate.cpp
@@ +64,5 @@
>  #define EXTRA_GC
>  #endif
>  
>  // GC will run once every thirty seconds during normal execution.
> +#define NORMAL_GC_TIMER_DELAY_MS 5000

nit: fix comment

@@ +69,3 @@
>  
>  // GC will run five seconds after the last event is processed.
> +#define IDLE_GC_TIMER_DELAY_MS 2000

nit: fix comment

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 743961 [details] [diff] [review]
Patch for mozilla-b2g18

Workers on b2g18 are not observing several of the important memory triggers specified in the preferences and are thus falling back to the JS engine runtime defaults (tuned for benchmarks on desktop). The result is that we almost never run GC during normal worker operation. The attached patch makes workers behave exactly like the main thread with respect to memory triggers.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Waiting for this to land on m-c, what are the risks of taking this to b2g18 branch?  How could qa verify this works as expected in builds?

Comment 15

[Justin Lebar (not reading bugmail)]

Most of our QA people seem unable to do anything but strict black-box testing, which is unsuitable for 95% of Gecko bugs (this one included).

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 743961 [details] [diff] [review]
Patch for mozilla-b2g18

09:45  bent> lsblakk, it's a branch-only thing
09:46  bent> lsblakk, we're going to need something pretty different on trunk
09:46  lsblakk> bent: so is there anything to help mitigate the risk of taking a unique patch to that branch? automated tests or known ways it might fail if there's an issue with the patch?
09:47  bent> lsblakk, well it affects the gc behavior of main thread as well as workers... so if anything goes wrong we'll know about it pretty fast either with crazy slow performance (which we'll see in QA I think) or with crashes (which we'll see in automated stability tests)

Based on this conversation we'll give this a go for v1-train landing since it should be obvious if the patch doesn't work as intended.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ryan, did this miss your uplift today?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

I don't see uplifts unless the bug is resolved. Use checkin-needed in situations like these. I'll get it in the morning.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Awesome, thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/6ebc1ed73336

Comment 21

[Ryan VanderMeulen [:RyanVM]]

And backed out for mochitest crashes. Why you gotta be that way, Ben?
https://hg.mozilla.org/releases/mozilla-b2g18/rev/55f6f8b52941

https://tbpl.mozilla.org/php/getParsedLog.php?id=22770347&tree=Mozilla-B2g18

Comment 22

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 747620 [details] [diff] [review]
patch

Thanks!

Comment 24

[Gregor Wagner [:gwagner]]

Attachment: Patch for mozilla-b2g18

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d5abab5ebc3c

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

bug 873356 tracks the mozilla-central work.