Closed Bug 1347815 Opened 3 years ago Closed 3 years ago

Label subclasses of nsExpirationTracker in layout modules

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bmo, Assigned: chenpighead)

References

Details

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

Attachments

(4 files)

+++ 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
Blocks: Layout-labeling
No longer blocks: Labeling
Severity: major → normal
Duplicate of this bug: 1345387
Summary: Label in layout modules → Label subclasses of nsExpirationTracker in layout modules
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]
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Attachment #8855726 - Flags: review?(cam)
Attachment #8855727 - Flags: review?(cam)
Attachment #8855728 - Flags: review?(cam)
Attachment #8855729 - Flags: review?(cam)
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 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 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+
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
You need to log in before you can comment on or make changes to this bug.