1347815 - Label subclasses of nsExpirationTracker in layout modules

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Astley Chen (inactive)]

+++ 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

Comment 2

[Cameron McCormack (:heycam)]

RuleProcessorCache should be able to use the SystemGroup, since it's a browser-wide cache.

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: Bug 1347815 - part1.1: fix some obvious nits for the implementations of ScrollFrameHelper.

Review commit: https://reviewboard.mozilla.org/r/127620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127620/

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: Bug 1347815 - part1.2: label ScrollFrameActivityTracker.

ScrollFrameActivityTracker::NotifyExpired() will be invoked by
nsExpirationTracker::TimerCallback() from an unlabel runnable.
We adopt the change in nsExpirationTracker to provide a DocGroup
EventTarget for the invocation of this callback.

Review commit: https://reviewboard.mozilla.org/r/127622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127622/

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1347815 - part2: label LayerActivityTracker.

LayerActivityTracker::NotifyExpired() will be invoked by
nsExpirationTracker::TimerCallback() from an unlabel runnable.
We adopt the change in nsExpirationTracker to provide a DocGroup
EventTarget for the invocation of this callback.

Review commit: https://reviewboard.mozilla.org/r/127624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127624/

Comment 7

[Jeremy Chen ([e-mail])]

Attachment: Bug 1347815 - part3: label RuleProcessorCache::mExpirationTracker.

RuleProcessorCache::mExpirationTracker.NotifyExpired() will be invoked by
nsExpirationTracker::TimerCallback() from an unlabel runnable.
Since RuleProcessorCache is a browser-wide cache, we provide a SystemGroup
EventTarget for the invocation of this callback.

Review commit: https://reviewboard.mozilla.org/r/127626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127626/

Comment 8

[Jeremy Chen ([e-mail])]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9a2b479e3bc0d3475456a7ab0be86950abc2c1e

Comment 9

[Cameron McCormack (:heycam)]

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

Comment 10

[Cameron McCormack (:heycam)]

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.

Comment 11

[Cameron McCormack (:heycam)]

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?

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8855728 [details]
Bug 1347815 - part2: label LayerActivityTracker.

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

Comment 13

[Cameron McCormack (:heycam)]

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.

Comment 14

[Markus Stange [:mstange]]

(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.

Comment 15

[Cameron McCormack (:heycam)]

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).

Comment 16

[Markus Stange [:mstange]]

(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.

Comment 17

[Cameron McCormack (:heycam)]

OK, sounds like SystemGroup is appropriate then.  Thanks!

Comment 18

[Jeremy Chen ([e-mail])]

Cameron and Markus, thank you for the explanation. I'll use SystemGroup to label ScrollFrameActivityTracker and LayerActivityTracker then.

Comment 19

[Jeremy Chen ([e-mail])]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127620/diff/1-2/

Comment 20

[Jeremy Chen ([e-mail])]

Comment on attachment 8855727 [details]
Bug 1347815 - part1.2: label ScrollFrameActivityTracker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127622/diff/1-2/

Comment 21

[Jeremy Chen ([e-mail])]

Comment on attachment 8855728 [details]
Bug 1347815 - part2: label LayerActivityTracker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127624/diff/1-2/

Comment 22

[Jeremy Chen ([e-mail])]

Comment on attachment 8855729 [details]
Bug 1347815 - part3: label RuleProcessorCache::mExpirationTracker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127626/diff/1-2/

Comment 23

[Cameron McCormack (:heycam)]

Comment on attachment 8855727 [details]
Bug 1347815 - part1.2: label ScrollFrameActivityTracker.

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

Comment 24

[Cameron McCormack (:heycam)]

Comment on attachment 8855728 [details]
Bug 1347815 - part2: label LayerActivityTracker.

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

Looks good, thanks!

Comment 25

[Jeremy Chen ([e-mail])]

Thank you for the review!
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=329e8f518d868461fe45013d25792b3a50e8252e

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

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