Closed
Bug 1130439
Opened 9 years ago
Closed 9 years ago
Investigate compacting the heap when the user is inactive
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
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)
15.40 KB,
patch
|
smaug
:
review+
terrence
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
||
Please test how videos behave with this. We don't want Flash or html videos to jank.
Assignee | ||
Comment 5•9 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•9 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•9 years ago
|
||
Attachment #8567895 -
Flags: review?(bugs)
Assignee | ||
Comment 9•9 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•9 years ago
|
Attachment #8567895 -
Flags: review?(bugs) → review+
Comment 10•9 years ago
|
||
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•9 years ago
|
Attachment #8566068 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6461410a7a
Comment 12•9 years ago
|
||
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•9 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.
Assignee | ||
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/95425fd7c738
https://hg.mozilla.org/mozilla-central/rev/95425fd7c738
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•