Commit 20a964e3 authored by Boris Chiou's avatar Boris Chiou
Browse files

Bug 1738135 - Don't create ScrollTimeline for each Animation. r=hiro 

We use a HashMap to keep the ScrollTimeline, and use scroll-direction as
the key, so a specific Element and a fixed scroll direction map to a specific
ScrollTimeline object. This means a maximum of one ScrollTimeline objects per
@scroll-timeline declaration.

If @scroll-timeline rule gets updated, we rebuild all the CSS
animations, and then re-check their ScrollTimeline objects. If there are
any unused old ScrollTimeline objects, their ref-counts will be zero
because only Animation object holds the strong reference,
so they will be removed from the associated ScrollTimelineSet and be
deleted automatically.

Note: We may have to update this once we support more descriptors in
@scroll-timeline. However, the syntax update is to obsolute @scroll-timeline,
so we don't have to address too many cases for now. This should be enough.

This is an internal optimization, and we can probably just rely on the existing
WPTs to test this without any memory leaks.

Differential Revision: https://phabricator.services.mozilla.com/D137236
parent ede63d09

dom/animation/ScrollTimeline.cpp

+26 −26
Original line number Diff line number Diff line
@@ -43,14 +43,15 @@ NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(ScrollTimeline, 


TimingParams ScrollTimeline::sTiming;



ScrollTimeline::ScrollTimeline(Document* aDocument, const Scroller& aScroller)

ScrollTimeline::ScrollTimeline(Document* aDocument, const Scroller& aScroller,

                               StyleScrollDirection aDirection)

    : AnimationTimeline(aDocument->GetParentObject()),

      mDocument(aDocument),

      // FIXME: Bug 1737918: We may have to udpate the constructor arguments

      // because this can be nearest, root, or a specific container. For now,

      // the input is a source element directly and it is the root element.

      mSource(aScroller),

      mDirection(StyleScrollDirection::Auto) {

      mDirection(aDirection) {

  MOZ_ASSERT(aDocument);



  // Use default values except for |mDuration| and |mFill|.
@@ -59,24 +60,34 @@ ScrollTimeline::ScrollTimeline(Document* aDocument, const Scroller& aScroller) 
  sTiming = TimingParams(SCROLL_TIMELINE_DURATION_MILLISEC, 0.0,

                         std::numeric_limits<float>::infinity(),

                         PlaybackDirection::Alternate, FillMode::Both);



  RegisterWithScrollSource();

}



already_AddRefed<ScrollTimeline> ScrollTimeline::FromRule(

    const RawServoScrollTimelineRule& aRule, Document* aDocument,

    const NonOwningAnimationTarget& aTarget) {

  // FIXME: Use ScrollingElement in the next patch.

  RefPtr<ScrollTimeline> timeline = new ScrollTimeline(

      aDocument, Scroller::Auto(aTarget.mElement->OwnerDoc()));



  // FIXME: Bug 1737918: applying new spec update.

  // Note: If the rules changes after we build the scroll-timeline rule, we

  // rebuild the animtions, so does the timeline object (because now we create

  // the scroll-timeline for each animation).

  // FIXME: Bug 1738135: If the scroll timeline is hold by Element, we have to

  // update it once the rule is changed.

  timeline->mDirection = Servo_ScrollTimelineRule_GetOrientation(&aRule);

  // rebuild all CSS animtions, and then try to look up the scroll-timeline by

  // the new source and the new direction. If we cannot find a specific

  // timeline, we create one, and the unused scroll-timeline object will be

  // dropped automatically becuase no animation owns it and its ref-count

  // becomes zero.



  StyleScrollDirection direction =

      Servo_ScrollTimelineRule_GetOrientation(&aRule);



  // FIXME: Bug 1737918: applying new spec update, e.g. other scrollers and

  // other style values.

  RefPtr<ScrollTimeline> timeline;

  auto autoScroller = Scroller::Auto(aTarget.mElement->OwnerDoc());

  auto* set =

      ScrollTimelineSet::GetOrCreateScrollTimelineSet(autoScroller.mElement);

  auto p = set->LookupForAdd(direction);

  if (!p) {

    timeline = new ScrollTimeline(aDocument, autoScroller, direction);

    set->Add(p, direction, timeline);

  } else {

    timeline = p->value();

  }

  return timeline.forget();

}
@@ -113,17 +124,6 @@ Nullable<TimeDuration> ScrollTimeline::GetCurrentTimeAsDuration() const { 
                                        SCROLL_TIMELINE_DURATION_MILLISEC);

}



void ScrollTimeline::RegisterWithScrollSource() {

  if (!mSource) {

    return;

  }



  if (ScrollTimelineSet* scrollTimelineSet =

          ScrollTimelineSet::GetOrCreateScrollTimelineSet(mSource.mElement)) {

    scrollTimelineSet->AddScrollTimeline(*this);

  }

}



void ScrollTimeline::UnregisterFromScrollSource() {

  if (!mSource) {

    return;
@@ -131,7 +131,7 @@ void ScrollTimeline::UnregisterFromScrollSource() { 


  if (ScrollTimelineSet* scrollTimelineSet =

          ScrollTimelineSet::GetScrollTimelineSet(mSource.mElement)) {

    scrollTimelineSet->RemoveScrollTimeline(*this);

    scrollTimelineSet->Remove(mDirection);

    if (scrollTimelineSet->IsEmpty()) {

      ScrollTimelineSet::DestroyScrollTimelineSet(mSource.mElement);

    }

dom/animation/ScrollTimeline.h

+25 −12
Original line number Diff line number Diff line
@@ -92,9 +92,6 @@ class ScrollTimeline final : public AnimationTimeline { 
    }

  };



  ScrollTimeline() = delete;

  ScrollTimeline(Document* aDocument, const Scroller& aScroller);



  // FIXME: Bug 1737918: Rewrite this because @scroll-timeline will be obsolete.

  static already_AddRefed<ScrollTimeline> FromRule(

      const RawServoScrollTimelineRule& aRule, Document* aDocument,
@@ -148,13 +145,16 @@ class ScrollTimeline final : public AnimationTimeline { 
  virtual ~ScrollTimeline() { Teardown(); }



 private:

  ScrollTimeline() = delete;

  ScrollTimeline(Document* aDocument, const Scroller& aScroller,

                 StyleScrollDirection aDirection);



  // Note: This function is required to be idempotent, as it can be called from

  // both cycleCollection::Unlink() and ~ScrollTimeline(). When modifying this

  // function, be sure to preserve this property.

  void Teardown() { UnregisterFromScrollSource(); }



  // Register/Unregister this scroll timeline to the element property.

  void RegisterWithScrollSource();

  // Unregister this scroll timeline to the element property.

  void UnregisterFromScrollSource();



  const nsIScrollableFrame* GetScrollFrame() const;
@@ -209,7 +209,16 @@ class ScrollTimeline final : public AnimationTimeline { 
 */

class ScrollTimelineSet {

 public:

  using NonOwningScrollTimelineSet = HashSet<ScrollTimeline*>;

  // Use StyleScrollDirection as the key, so we reuse the ScrollTimeline with

  // the same source and the same direction.

  // Note: the drawback of using the direction as the key is that we have to

  // update this once we support more descriptors. This implementation assumes

  // scroll-offsets will be obsolute. However, I'm pretty sure @scroll-timeline

  // will be obsolute, based on the spec issue. We may have to do a lot of

  // updates after the spec updates, so this tentative implmentation should be

  // enough for now.

  using NonOwningScrollTimelineMap =

      HashMap<StyleScrollDirection, ScrollTimeline*>;



  ~ScrollTimelineSet() = default;
@@ -217,18 +226,20 @@ class ScrollTimelineSet { 
  static ScrollTimelineSet* GetOrCreateScrollTimelineSet(Element* aElement);

  static void DestroyScrollTimelineSet(Element* aElement);



  void AddScrollTimeline(ScrollTimeline& aScrollTimeline) {

    Unused << mScrollTimelines.put(&aScrollTimeline);

  NonOwningScrollTimelineMap::AddPtr LookupForAdd(StyleScrollDirection aKey) {

    return mScrollTimelines.lookupForAdd(aKey);

  }

  void RemoveScrollTimeline(ScrollTimeline& aScrollTimeline) {

    mScrollTimelines.remove(&aScrollTimeline);

  void Add(NonOwningScrollTimelineMap::AddPtr& aPtr, StyleScrollDirection aKey,

           ScrollTimeline* aScrollTimeline) {

    Unused << mScrollTimelines.add(aPtr, aKey, aScrollTimeline);

  }

  void Remove(StyleScrollDirection aKey) { mScrollTimelines.remove(aKey); }



  bool IsEmpty() const { return mScrollTimelines.empty(); }



  void ScheduleAnimations() const {

    for (auto iter = mScrollTimelines.iter(); !iter.done(); iter.next()) {

      iter.get()->ScheduleAnimations();

      iter.get().value()->ScheduleAnimations();

    }

  }
@@ -241,9 +252,11 @@ class ScrollTimelineSet { 
  // The ScrollTimeline is generated only by CSS, so if all the associated

  // Animations are gone, we don't need the ScrollTimeline anymore, so

  // ScrollTimelineSet doesn't have to keep it for the source element.

  // We rely on ScrollTimeline::Teardown() to remove the unused ScrollTimeline

  // from this hash map.

  // FIXME: Bug 1676794: We may have to update here if it's possible to create

  // ScrollTimeline via script.

  NonOwningScrollTimelineSet mScrollTimelines;

  NonOwningScrollTimelineMap mScrollTimelines;

};



}  // namespace dom