Investigate compacting the heap when the user is inactive

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8560517 [details] [diff] [review]
compact-when-user-inactive WIP

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
(Assignee)

Comment 2

3 years ago
Created attachment 8566068 [details] [diff] [review]
compact-when-user-inactive

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 3

3 years ago
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+

Comment 4

3 years ago
Please test how videos behave with this. We don't want Flash or html videos to jank.
(Assignee)

Comment 5

3 years ago
(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)

Comment 6

3 years ago
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)
(Assignee)

Comment 8

3 years ago
Created attachment 8567895 [details] [diff] [review]
compact-when-user-inactive v2
Attachment #8567895 - Flags: review?(bugs)
(Assignee)

Comment 9

3 years ago
Comment on attachment 8567895 [details] [diff] [review]
compact-when-user-inactive v2

Requesting review for JS changes.
Attachment #8567895 - Flags: review?(terrence)

Updated

3 years ago
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+
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 13

3 years ago
(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
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

3 years ago
Depends on: 1170034
(Assignee)

Updated

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