1230056 - Add EffectCompositor::HasAnimationsForCompositor()

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles)]

As suggested in bug 1226118 comment 31.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Add EffectCompositor::HasAnimationsForCompositor()

This saves us allocating an array only to throw it away again for cases
where we are only interested in the presence or absence of compositor
animations.

This patch also remove nsLayoutUtils::HasAnimationsForCompositor since it
is no longer needed (it would simply wrap this function if we kept it).


I tried to make the aOnMatch argument optional using Maybe<> but it turned out
to be very complicated. Basically, if we pass a lambda function to Some(), the
template argument of Maybe<> would get resolved to something that doesn't
match the inner Function<> type of aOnMatch. To work around that we'd basically
have to pass Maybe<Function<void(Animation&)>>().emplace(...lambda...) which
is much more work than simply passing an empty lambda in the case where we don't
need aOnMatch.

If we change aOnMatch to a template type, we'd avoid that problem but then
we can't pass Nothing() since it won't resolve to a type that we could
potentially call using operator().

Basically, at the end of the day it seems simplest to just pass an empty
function for the case where we don't need to do anything in aOnMatch.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8695590 [details] [diff] [review]
Add EffectCompositor::HasAnimationsForCompositor()

Review of attachment 8695590 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good; nits below.

Also: if you like, you could split this into two pieces:
 Part (1) would be basically non-functional, & would move the current nsLayoutUtils impl directly to EffectCompositor, without changing it (no lambda stuff yet), and would update all the callsites.
 Part (2) would do the code-refactoring inside of EffectCompositor (only touching that file) -- this is where the lambda-based code-sharing would happen.

r=me either way (feel free, but not obligated, to re-request review if you want to make that change & would like another sanity-check)

::: dom/animation/EffectCompositor.cpp
@@ +27,5 @@
> +//
> +// Returns true if there are eligible animations, false otherwise.
> +//
> +// NOTE: It is important to check the return value of this function because
> +// even if aOnMatch is called whilst iterating over animations, a subsequent

Before "because", could you add something like "(and disregard any matches reported via aOnMatch if false is returned)"?

Otherwise, it's a bit unclear what you're talking about here, RE "It is important to check the return value of this function". (Check for what? What's the alternative to checking that we're worried about?)

(Further down, you do mention "the caller can ignore any prior calls to aOnMatch", but that feels like an afterthought -- and really it's a key part of what you're warning about here.)

@@ +89,5 @@
> +  bool foundSome =
> +    FindAnimationsForCompositor(aFrame, aProperty,
> +      [&result](Animation& aAnimation)
> +      {
> +        result.AppendElement(&aAnimation);

I'd rather we make this lambda take "Animation* aAnimation" and call result.AppendElement(aAnimation).

Reasons:
* Feels a bit wrong to be passing around a C++ reference to a refcounted heap object.
* This is a pointer in the caller; might as well keep it a pointer here, too, for consistency.
* We kind of know it's not null, but we haven't really explicitly checked that in the caller.
* Right now the caller has to explicitly deref its pointer to pass in a reference, and this lambda has to explicitly get the address. Simpler to just pass the pointer in, to save a "*" at the callsite and a "&" here.

Feel free to add MOZ_ASSERT(aAnimation) here, if you want to document that we know this value is non-null.

Comment 3

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> ::: dom/animation/EffectCompositor.cpp
> @@ +27,5 @@
> > +//
> > +// Returns true if there are eligible animations, false otherwise.
> > +//
> > +// NOTE: It is important to check the return value of this function because
> > +// even if aOnMatch is called whilst iterating over animations, a subsequent
> 
> Before "because", could you add something like "(and disregard any matches
> reported via aOnMatch if false is returned)"?
> 
> Otherwise, it's a bit unclear what you're talking about here, RE "It is
> important to check the return value of this function". (Check for what?
> What's the alternative to checking that we're worried about?)
> 
> (Further down, you do mention "the caller can ignore any prior calls to
> aOnMatch", but that feels like an afterthought -- and really it's a key part
> of what you're warning about here.)

Will do, thanks!

> @@ +89,5 @@
> > +  bool foundSome =
> > +    FindAnimationsForCompositor(aFrame, aProperty,
> > +      [&result](Animation& aAnimation)
> > +      {
> > +        result.AppendElement(&aAnimation);
> 
> I'd rather we make this lambda take "Animation* aAnimation" and call
> result.AppendElement(aAnimation).
> 
> Reasons:
> * Feels a bit wrong to be passing around a C++ reference to a refcounted
> heap object.

What's the problem? Do we have tooling that only detects dangling pointers and not dangling references?

Otherwise, a raw pointer doesn't seem any better here? (It seems worse since it doesn't document that we expect the thing to never be null).

> * This is a pointer in the caller; might as well keep it a pointer here,
> too, for consistency.

I think it's better for the caller to be responsible for checking if this thing can be null or not. The caller has knowledge about if that's possible or not. We shouldn't require every call site to do that work or have that knowledge. If the thing could be null, then the call site should check it and not call aOnMatch in that case.

> * We kind of know it's not null, but we haven't really explicitly checked
> that in the caller.

I'm pretty sure we do here:

  MOZ_ASSERT(effect && effect->GetAnimation());

> * Right now the caller has to explicitly deref its pointer to pass in a
> reference, and this lambda has to explicitly get the address. Simpler to
> just pass the pointer in, to save a "*" at the callsite and a "&" here.

Fair enough, but I think it's just a coincidence that the callback function happens to use a pointer. From an API design point of view, it seems more useful to pass a reference so the caller knows this parameter can never be null. Otherwise, any time someone interacts with this function they have to trace the possible call stacks and make sure the thing can't be null.

In general I think we should use references more regularly as this seems to be generally agreed-upon best practice[1] and typically results in less code (no unnecessary null-checks including assertions) and more robust code (no forgetting to check if a value could be null). Of course there are exceptions like where we use pointers for things like nsPresContext, nsIFrame etc. for consistency reasons, as well as for out-params.

Anyway, I'll do what you suggested but I thought it was worth bringing it up since I hope we can eventually tidy up our code in this regard. We did this at a previous company and the difference to code readability and robustness was incredible.

[1] https://isocpp.org/wiki/faq/references#refs-vs-ptrs

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 1 - Add EffectCompositor::HasAnimationsForCompositor

Hi Daniel, I've rewritten this to fix the performance issues in bug 1230448.
Would you mind re-reviewing?

It turns out we really need to handle the no animations case first. Also,
I bumped the check for frames refusing async animations up too since the call
to nsLayoutUtils::AreAsyncAnimationsEnabled() if quite costly (it checks prefs,
which, at least on the first time they are queried, can be expensive).

Also, trying to refactor the code into a single implementation with an optional
callback function proved to introduce further performance problems although
I'm not entirely sure why--mozilla::Function doesn't seem so expensive and
I would have expected that calling an empty function in the
HasAnimationsForCompositor case would have been reasonably cheap too.

In any case, I've reworked it so much of the checks are in
HasAnimationsForCompositor and GetAnimationsForCompositor simply calls that.
That introduces some redundancy which is unfortunate, but I think it's
acceptable for the performance benefits.

I've also split out the changes where I dropped
nsLayoutUtils::HasAnimationsForCompositor into a separate patch as you
suggested.

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 2 - Remove nsLayoutUtils::HasAnimationsForCompositor and call EffectCompositor::HasAnimationsForCompositor directly

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #3)
> > Reasons:
> > * Feels a bit wrong to be passing around a C++ reference to a refcounted
> > heap object.
> 
> What's the problem? Do we have tooling that only detects dangling pointers
> and not dangling references?

(1) Partly just a consistent-coding-style thing. AFAIK, we universally deal with refcounted objects as pointers rather than as C++ references. (It sounds like you'd like to change that, but we should probably get some consensus on that before shifting away from current convention.)

(2) Partly because there are several things which are natural for us to do with refcounted objects, all of which are natural to do with a pointer but unnatural with a C++ reference:
 - Increment the refcount (via instantiating a nsRefPtr)
 - Give up our ownership to some other owner (via forget()/already_AddRefed) 
 - Pass the object to a function, as a pointer.

If we've got a C++ reference, these actions are all at least somewhat unnatural to do, and can lead to e.g. dangling-reference bugs if we pass off our ownership while still having a C++ reference that we have no way of invalidating.  If we use a pointer, these all Just Work.

> Anyway, I'll do what you suggested but I thought it was worth bringing it up

Cool - this may merit more discussion off-bug, but for now I think it's good to stick with the current gecko conventions.  This sounds like a good candidate for a dev.platform discussion

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8696513 [details] [diff] [review]
part 2 - Remove nsLayoutUtils::HasAnimationsForCompositor and call EffectCompositor::HasAnimationsForCompositor directly

Review of attachment 8696513 [details] [diff] [review]:
-----------------------------------------------------------------

Starting with part 2 (the more mechanical easy-to-review part): r=me

(I'm hoping to review part 1 during some downtime today.)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8696512 [details] [diff] [review]
part 1 - Add EffectCompositor::HasAnimationsForCompositor

Review of attachment 8696512 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/EffectCompositor.cpp
@@ +71,5 @@
> +
> +  // HasAnimationsForCompositor performs additional checks not covered here
> +  // so we need to call it first. (Implementing both functions using the
> +  // a generic function with a callback argument proved to be too slow so
> +  // there is someoverlap in the logic in these two functions.)

It seems to me like we could still use a generic function here (and share logic more cleanly, and avoid redundant looping) by converting this patch's version of HasAnimationsForCompositor into a generic function, with its final loop calling a lambda function instead of setting |foundSome = true|.

Wouldn't that work? Or am I missing something?

@@ +78,5 @@
> +  }
> +
> +  EffectSet* effects = EffectSet::GetEffectSet(aFrame);
> +  MOZ_ASSERT(effects && !effects->IsEmpty(),
> +             "HasAnimationsForCompositor should not true for missing or "

missing a word here -- "should not _return_ true"

Comment 9

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> > +  // HasAnimationsForCompositor performs additional checks not covered here
> > +  // so we need to call it first. (Implementing both functions using the
> > +  // a generic function with a callback argument proved to be too slow so
> > +  // there is someoverlap in the logic in these two functions.)
> 
> It seems to me like we could still use a generic function here (and share
> logic more cleanly, and avoid redundant looping) by converting this patch's
> version of HasAnimationsForCompositor into a generic function, with its
> final loop calling a lambda function instead of setting |foundSome = true|.
> 
> Wouldn't that work? Or am I missing something?

I think it would work, but I'm not sure what the performance characteristics would be. I'm not entirely sure what was introducing the performance regression from factoring it out into generic function so I'll have to try it and see.

Comment 10

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> It seems to me like we could still use a generic function here (and share
> logic more cleanly, and avoid redundant looping) by converting this patch's
> version of HasAnimationsForCompositor into a generic function, with its
> final loop calling a lambda function instead of setting |foundSome = true|.
> 
> Wouldn't that work? Or am I missing something?

Actually, sorry, that doesn't work. I misunderstood what you were referring to. I tried doing that and it introduces a 2% perf regression (presumably from calling the empty lambda function even for the HasAnimations case).

Comment 11

[Daniel Holbert [:dholbert]]

OK. In that case, I'd expect we could just use a non-lambda-dependent helper-function like:

 bool
 FindAnimationsForCompositor(..., nsTArray<RefPtr<dom::Animation>>* aArray /*outparam*/, ...)

The HasAnimationsForCompositor function would pass in a null outparam, and FindAnimationsForCompositor would pass in a non-null outparam. And internally, FindAnimationsForCompositor would append to aArray iff it's non-null (and would return a bool indicating whether it found anything, regardless).

The null-check shouldn't make us fall off of the processor's fast path like a lambda-invocation would, I'd expect.

Comment 12

[Daniel Holbert [:dholbert]]

(And this would still have the benefit of avoiding repeated loops performing repeated logic, which I think is a worthy goal, and I'd be surprised if the added null-check would cause anything like the 2% perf regression. I can easily imagine lambdas causing that in a way that a null-check would not.)

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 1 - Add EffectCompositor::HasAnimationsForCompositor

Comment 14

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> (And this would still have the benefit of avoiding repeated loops performing
> repeated logic, which I think is a worthy goal, and I'd be surprised if the
> added null-check would cause anything like the 2% perf regression. I can
> easily imagine lambdas causing that in a way that a null-check would not.)

Yeah, I had the same idea (and thought I mentioned it but just now noticed the mid-air collision!)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8696997 [details] [diff] [review]
part 1 - Add EffectCompositor::HasAnimationsForCompositor

Review of attachment 8696997 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Just one nit. Also, I only looked at the EffectCompositor.cpp changes in this patch; let me know if anything else needs re-review here.

::: dom/animation/EffectCompositor.cpp
@@ +78,4 @@
>    }
>  
> +  MOZ_ASSERT(!foundSome || !aMatches || !aMatches->IsEmpty(),
> +             "If return value is true, matches array should be non-empty");

This assertion should probably be semi-copied (or move in its entirety) to the callsite in GetAnimationsForCompositor, to be sure that we don't violate this post-condition via any of our early returns.

Comment 16

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> ::: dom/animation/EffectCompositor.cpp
> @@ +78,4 @@
> >    }
> >  
> > +  MOZ_ASSERT(!foundSome || !aMatches || !aMatches->IsEmpty(),
> > +             "If return value is true, matches array should be non-empty");
> 
> This assertion should probably be semi-copied (or move in its entirety) to
> the callsite in GetAnimationsForCompositor, to be sure that we don't violate
> this post-condition via any of our early returns.

Makes sense. Thanks!

Perfherder results look good too:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=66e520cba75b&newProject=try&newRevision=902bc57e9a60&showOnlyImportant=0

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c97313b5857
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff143ef49eb

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2c97313b5857
https://hg.mozilla.org/mozilla-central/rev/6ff143ef49eb