Label subclasses of nsExpirationTracker in layout modules

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: astley, Assigned: jeremychen)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [QDL][TDC-MVP][LAYOUT])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

9 months ago
+++ This bug was initially created as a clone of Bug #1345387 comment 1+++

There is a bunch of use cases of nsExpirationTracker under layout/* folders.
In bug 1345464, an optional EventTarget will be applied to nsExpirationTracker so that it's gonna be a lot easier to label such high frequency runnables in subclass of nsExpirationTracker.

This bug is targeting to resolve all these cases in layout module.

http://searchfox.org/mozilla-central/search?q=nsExpirationTracker&case=false&regexp=false&path=layout

layout/generic/nsGfxScrollFrame.cpp
layout/painting/ActiveLayerTracker.cpp
layout/style/RuleProcessorCache.h
(Reporter)

Updated

9 months ago
Blocks: 1339343
No longer blocks: 1321812
Severity: major → normal
(Reporter)

Updated

9 months ago
Duplicate of this bug: 1345387
(Reporter)

Updated

9 months ago
Summary: Label in layout modules → Label subclasses of nsExpirationTracker in layout modules
(Reporter)

Updated

9 months ago
Blocks: 1338446
RuleProcessorCache should be able to use the SystemGroup, since it's a browser-wide cache.
Blocks: 1342870
Duplicate of this bug: 1342870
Whiteboard: [QDL][TDC-MVP][LAYOUT]
(Reporter)

Updated

8 months ago
Assignee: nobody → jeremychen
(Reporter)

Updated

8 months ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8855726 - Flags: review?(cam)
Attachment #8855727 - Flags: review?(cam)
Attachment #8855728 - Flags: review?(cam)
Attachment #8855729 - Flags: review?(cam)
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9a2b479e3bc0d3475456a7ab0be86950abc2c1e

Comment 9

8 months ago
mozreview-review
Comment on attachment 8855726 [details]
Bug 1347815 - part1.1: fix some obvious nits for the implementations of ScrollFrameHelper.

https://reviewboard.mozilla.org/r/127620/#review130544
Attachment #8855726 - Flags: review?(cam) → review+
Comment on attachment 8855727 [details]
Bug 1347815 - part1.2: label ScrollFrameActivityTracker.

https://reviewboard.mozilla.org/r/127622/#review130546

::: commit-message-79dd1:4
(Diff revision 1)
> +Bug 1347815 - part1.2: label ScrollFrameActivityTracker.
> +
> +ScrollFrameActivityTracker::NotifyExpired() will be invoked by
> +nsExpirationTracker::TimerCallback() from an unlabel runnable.

unlabeled

::: layout/generic/nsGfxScrollFrame.cpp:2543
(Diff revision 1)
> -      gScrollFrameActivityTracker = new ScrollFrameActivityTracker();
> +      gScrollFrameActivityTracker = new ScrollFrameActivityTracker(
> +        mScrolledFrame->GetContent()->OwnerDoc()->EventTargetFor(
> +          TaskCategory::Other));

I don't think this is right -- gScrollFrameActivityTracker is a global variable that gets created here the first time it is needed (and deleted if it becomes empty, in ScrollFrameHelper::Destroy).  So it's not the case that all entries in the tracker are related to this document you're passing in.  Same for LayerActivityTracker.
Attachment #8855727 - Flags: review?(cam) → review-
So my feeling is that ScrollFrameActivityTracker and LayerActivityTracker could probably both use SystemGroup, since there's nothing within a page that would rely on the timer firing at a particular time (i.e., it doesn't matter when this timer's callback is scheduled, relative to other runnables dispatched for the page) and don't have an effect on layout.  Markus, would you agree with that?
Flags: needinfo?(mstange)
Comment on attachment 8855728 [details]
Bug 1347815 - part2: label LayerActivityTracker.

https://reviewboard.mozilla.org/r/127624/#review130550
Attachment #8855728 - Flags: review?(cam)
Comment on attachment 8855729 [details]
Bug 1347815 - part3: label RuleProcessorCache::mExpirationTracker.

https://reviewboard.mozilla.org/r/127626/#review130552

::: commit-message-56d06:4
(Diff revision 1)
> +Bug 1347815 - part3: label RuleProcessorCache::mExpirationTracker.
> +
> +RuleProcessorCache::mExpirationTracker.NotifyExpired() will be invoked by
> +nsExpirationTracker::TimerCallback() from an unlabel runnable.

unlabeled

::: layout/style/RuleProcessorCache.h:73
(Diff revision 1)
> -    explicit ExpirationTracker(RuleProcessorCache* aCache)
> +    explicit ExpirationTracker(RuleProcessorCache* aCache,
> +                               nsIEventTarget* aEventTarget)
>        : nsExpirationTracker<nsCSSRuleProcessor,3>(
> -          10000, "RuleProcessorCache::ExpirationTracker")
> +          10000, "RuleProcessorCache::ExpirationTracker", aEventTarget)

Might be simpler just to call SystemGroup::EventTargetFor(...) here, rather than in RuleProcessCache's constructor and pass it up to ExpirationTracker.
Attachment #8855729 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #11)
> there's nothing within a page
> that would rely on the timer firing at a particular time (i.e., it doesn't
> matter when this timer's callback is scheduled, relative to other runnables
> dispatched for the page) and don't have an effect on layout.

I agree with this.
I don't know if this means that SystemGroup is the appropriate group for these timers. These timers can have a lower priority than everything else and still nothing would break.
Flags: needinfo?(mstange)
Thanks.  So that makes me wonder: there is a user visible effect to these timers running, e.g. you can notice that text gets re-rendered with sub-pixel AA when an element is delayerized.  If it's possible that using SystemGroup would substantially or significantly delay the firing of the timer, then that might not be the effect we want to see?  If so, would it be too much overhead to have one of these trackers per document?  That might not be great in terms of the number of timers firing at different times (whereas now we're effectively coalescing them all into one timer).
(In reply to Cameron McCormack (:heycam) from comment #15)
> there is a user visible effect to these
> timers running, e.g. you can notice that text gets re-rendered with
> sub-pixel AA when an element is delayerized.

That's true. The main reason for these timers is to avoid run-away memory usage as more and more layers become active.

> If it's possible that using
> SystemGroup would substantially or significantly delay the firing of the
> timer, then that might not be the effect we want to see?

I don't think we need to worry about this until we see reports of it causing problems.

De-layerizing causes repaints, and if the process is so busy that these timers are significantly delayed, then we probably have better things to do than to spend time on repaints whose only purpose is memory reduction.
OK, sounds like SystemGroup is appropriate then.  Thanks!
Cameron and Markus, thank you for the explanation. I'll use SystemGroup to label ScrollFrameActivityTracker and LayerActivityTracker then.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8855727 [details]
Bug 1347815 - part1.2: label ScrollFrameActivityTracker.

https://reviewboard.mozilla.org/r/127622/#review130646
Attachment #8855727 - Flags: review?(cam) → review+
Comment on attachment 8855728 [details]
Bug 1347815 - part2: label LayerActivityTracker.

https://reviewboard.mozilla.org/r/127624/#review130648

Looks good, thanks!
Attachment #8855728 - Flags: review?(cam) → review+
Thank you for the review!
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=329e8f518d868461fe45013d25792b3a50e8252e

Comment 26

8 months ago
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9152febfbebb
part1.1: fix some obvious nits for the implementations of ScrollFrameHelper. r=heycam
https://hg.mozilla.org/integration/autoland/rev/59e4d3a76870
part1.2: label ScrollFrameActivityTracker. r=heycam
https://hg.mozilla.org/integration/autoland/rev/945099e38fad
part2: label LayerActivityTracker. r=heycam
https://hg.mozilla.org/integration/autoland/rev/664149a5ed24
part3: label RuleProcessorCache::mExpirationTracker. r=heycam

Comment 27

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9152febfbebb
https://hg.mozilla.org/mozilla-central/rev/59e4d3a76870
https://hg.mozilla.org/mozilla-central/rev/945099e38fad
https://hg.mozilla.org/mozilla-central/rev/664149a5ed24
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.