Closed Bug 1130439 Opened 5 years ago Closed 5 years ago

Investigate compacting the heap when the user is inactive

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

njn noticed that enabling compacting GC didn't make any difference on AWSY.  I think the problem might be that we don't actually compact except for memory pressure notifications and periodic full GC (which runs once a minute but dependent on several memory level heuristics).

This bug is to investigate compacting when the user goes inactive to see if that makes a difference.
Attached patch compact-when-user-inactive WIP (obsolete) — Splinter Review
Work in progress.  This seems to trigger bug 1127696 as well as give a couple of other oranges on try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da5c6cb94bcf
Attached patch compact-when-user-inactive (obsolete) — Splinter Review
This patch adds another time for shrinking GCs, started when the user goes idle.  I set time timeout for 15 seconds, which in addition to the 5 seconds for idleness to be recognised gives 20 seconds.  So the aim is that this doesn't happen every time the user stops moving the mouse while they're reading.

On the JS side we also record the last animation time in the runtime so we can skip any requested compacting while we are animating.

Smaug, I think this covers what we discussed.  Does this look ok to you?
Attachment #8560517 - Attachment is obsolete: true
Attachment #8566068 - Flags: feedback?(bugs)
Comment on attachment 8566068 [details] [diff] [review]
compact-when-user-inactive

>+void
>+ShrinkingGCTimerFired(nsITimer *aTimer, void *aClosure)
Nit, * goes with the type

>-    return invocationKind == GC_SHRINK && isCompactingGCEnabled();
>+    // Compact on shrinking GC if enabled, but skip compacting in incremental
>+    // GCs if we are currently animiating.
animiating?
Attachment #8566068 - Flags: feedback?(bugs) → feedback+
Please test how videos behave with this. We don't want Flash or html videos to jank.
(In reply to Olli Pettay [:smaug] (no new review requests before Feb 22, please) from comment #4)

This works for JS canvas animations and I've verified that we skip compacting in that case, but for youtube videos using the html5 player it still compacts (although I didn't see any jank).

This is using js::NotifyAnimationActivity() API to detect when animation is happening, which is called from two places in the browser: nsGlobalWindow::RequestAnimationFrame() and HTMLCanvasElement::InvalidateCanvasContent().

roc, do you know if there are other situations where we can be animating in the browser where this API doesn't get called?
Flags: needinfo?(roc)
The question is also, does it matter it we compact while showing a video, or does video just
bypass all main thread painting etc code? Same for plugins
(In reply to Olli Pettay [:smaug] (no new review requests before Feb 22, please) from comment #4)
> Please test how videos behave with this. We don't want Flash or html videos
> to jank.

Video decoding and playback is already independent of the main thread. If you find a situation where it's not, please file a bug. Ditto for plugins.
Flags: needinfo?(roc)
Attachment #8567895 - Flags: review?(bugs)
Comment on attachment 8567895 [details] [diff] [review]
compact-when-user-inactive v2

Requesting review for JS changes.
Attachment #8567895 - Flags: review?(terrence)
Attachment #8567895 - Flags: review?(bugs) → review+
Comment on attachment 8567895 [details] [diff] [review]
compact-when-user-inactive v2

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

Great!
Attachment #8567895 - Flags: review?(terrence) → review+
Attachment #8566068 - Attachment is obsolete: true
Backed out for causing the SM slowdowns/failures that originally got bug 1047529 backed out. I did a Try run of each of the bugs included in Jon's pushed backed out individually and this was the one that restored behavior to normal. The easiest thing to observe is that this patch caused the tests to take a massive slowdown (like 2-3x). That should be visible on a Try push with or without any other failures.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e26365be89aa
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12)
The problem is a jstest timeout that happens nearly all the time with this patch applied.  The tests hang for an hour towards the end, so this is not a slowdown.

Note that this patch mainly contains mainly browser changes that aren't built in the shell and the shell changes only have the effect of skipping compacting GC when we are animating, a situation that doesn't occur in the shell either.
https://hg.mozilla.org/mozilla-central/rev/95425fd7c738
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1170034
Depends on: 1188834
You need to log in before you can comment on or make changes to this bug.