1399049 - stylo: do not use InterpolateMatrix as a fallback for interpolation errors in a matched transform function pair

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P4)

Description

[Jeremy Chen ([e-mail])]

In Bug 1394284, if there is any interpolation error in a matched transform function pair, it fall-back to "InterpolateMatrix" (which uses more memory and calculation), and leave the handle of discrete animation to ```Servo_MatrixTransform_Operate()```. However, we should be able to just return Err() in Animate of TransformList, and use the handle of discrete animation in ```Servo_AnimationCompose``` [1].

This might need a little bit refactoring work.

One approach could be changing the logic in Animate of TransformList by replacing the current one pass logic with two passes. We can scan for the mismatched transform function pair for the first one pass, and then scan a second pass for the matched transform function pair to see if there's any interpolation error. The time complexity should be fine, more importantly, this could save the memory and calculation.



[1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/servo/ports/geckolib/glue.rs#553-559

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched transform function pair.

In the current implementation, if there is any interpolation error in a matched
transform function pair, we fall-back to use InterpolateMatrix unconditionally.
However, the error could be caused by:

1. mismatched transform function pair
2. matched transform function pair within at least one undecomposable matrix.

Using InterpolateMatrix for case 1 makes sense, however, using InterpolateMatrix
for case 2 does not. According to the spec, we should just report error for
case 2, and let the caller do the fallback procedure. Using InterpolateMatrix
for case 2 will go through more unnecessary code path, and produce more memory
usage and calculation cost, which should be avoidable.

In this patch, we add an extra pass to check if a transform function pair have
matched operations in advance. With this information, we can easily tell whether
the interpolation error in a equal-length transform function pair is caused by
case 1 or case 2. So, we can avoid the unnecessary cost.

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

Comment 2

[Jeremy Chen ([e-mail])]

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8919685 [details]
Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched transform function pair.

https://reviewboard.mozilla.org/r/190600/#review196054

This looks good to me for now. But I am wondering, once we introduced more TransformOperation types in bug 1398038, how does is_matched_oparation looks like?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1162
(Diff revision 1)
>              _ => Err(()),
>          }
>      }
>  }
>  
> +fn is_matched_operation(first: &TransformOperation, second: &TransformOperation) -> Result<(), ()> {

I wonder why this function does not return boolean instead of Resule<>.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2170
(Diff revision 1)
>          {
>              let this = (*this).0.as_ref().map_or(&[][..], |l| l);
>              let other = (*other).0.as_ref().map_or(&[][..], |l| l);
>              if this.len() == other.len() {
> +                let mut is_matched_transforms = true;
> +                for (this_op, other_op) in this.iter().zip(other.iter()) {

Can we use zip().all() if is_matched_operation?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2187
(Diff revision 1)
> -                    return Ok(TransformList(if list.is_empty() {
> +                        return Ok(TransformList(if list.is_empty() {
> -                        None
> +                            None
> -                    } else {
> +                        } else {
> -                        Some(list)
> +                            Some(list)
> -                    }));
> +                        }));
> +                    } else {

I don't think this else block is necessary.  We can write 

if let Ok(list) = result {
  return ..
}

return Err(());


Or simply, let result = ...()?;

Comment 4

[Jeremy Chen ([e-mail])]

Comment on attachment 8919685 [details]
Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched transform function pair.

https://reviewboard.mozilla.org/r/190600/#review196054

Thank you for the review.

Not sure what do you mean by more TransformOperation types, I only see CompositeOperations in bug 1398038, which should be animation procedures (e.g., "replace", "add", "accumulate") not TransformOperations, no?

Even if we'd like to add more TransformOperation types in future, I guess we'll need to fix TransformOperation's Animate implementation [1] anyway. When that time comes, we can fix is_matched_oparation() in the same way I think.


[1] https://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/servo/components/style/properties/helpers/animated_properties.mako.rs#1082

> I wonder why this function does not return boolean instead of Resule<>.

Yep, you're right. No need to use Resule<> here. Will fix this.

> Can we use zip().all() if is_matched_operation?

Don't know this such a good feature until now. Thank you. :)

Comment 5

[Jeremy Chen ([e-mail])]

Comment on attachment 8919685 [details]
Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched transform function pair.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from comment #4)
> Comment on attachment 8919685 [details]
> Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched
> transform function pair.
> 
> https://reviewboard.mozilla.org/r/190600/#review196054
> 
> Thank you for the review.
> 
> Not sure what do you mean by more TransformOperation types, I only see
> CompositeOperations in bug 1398038, which should be animation procedures
> (e.g., "replace", "add", "accumulate") not TransformOperations, no?
> 
> Even if we'd like to add more TransformOperation types in future, I guess
> we'll need to fix TransformOperation's Animate implementation [1] anyway.
> When that time comes, we can fix is_matched_oparation() in the same way I
> think.

Oops, I meant bug 1391145.

Comment 7

[Jeremy Chen ([e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from
> comment #4)
> > Comment on attachment 8919685 [details]
> > Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched
> > transform function pair.
> > 
> > https://reviewboard.mozilla.org/r/190600/#review196054
> > 
> > Thank you for the review.
> > 
> > Not sure what do you mean by more TransformOperation types, I only see
> > CompositeOperations in bug 1398038, which should be animation procedures
> > (e.g., "replace", "add", "accumulate") not TransformOperations, no?
> > 
> > Even if we'd like to add more TransformOperation types in future, I guess
> > we'll need to fix TransformOperation's Animate implementation [1] anyway.
> > When that time comes, we can fix is_matched_oparation() in the same way I
> > think.
> 
> Oops, I meant bug 1391145.

Ok, I know what you were saying now. So, with introducing more TransformOperation types bug 1391145, we may cause more unnecessary cases to fall into mismatched transform lists case. Though this won't regress the current behavior, but more like we gain some performance (in matched but with non-invertible matrix) in this bug and lose some more (between matched primitives) in bug 1391145.

However, with or without my patch in this bug, we'll have to face the issue anyway once we land bug 1391145, right? So, I think the issue should not block us from landing this bug.

We should probably fix the issue in bug 1391145, or file a followup for that. If we choose the latter, I'm happy to pick up the followup. One approach in my mind is, add a to_primitive() helper function to make operations like translate{x,y,z}, scale{x,y,z}, to become shared primitives (translate, scale), before doing animation.

Hi hiro, WDYT?

Comment 8

[Jeremy Chen ([e-mail])]

try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=265c74cd2886334aa79a07d472ff4a363613414a

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > (In reply to Jeremy Chen [:jeremychen] UTC+8 (away 24 Oct – 12 Nov) from
> > comment #4)
> > > Comment on attachment 8919685 [details]
> > > Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched
> > > transform function pair.
> > > 
> > > https://reviewboard.mozilla.org/r/190600/#review196054
> > > 
> > > Thank you for the review.
> > > 
> > > Not sure what do you mean by more TransformOperation types, I only see
> > > CompositeOperations in bug 1398038, which should be animation procedures
> > > (e.g., "replace", "add", "accumulate") not TransformOperations, no?
> > > 
> > > Even if we'd like to add more TransformOperation types in future, I guess
> > > we'll need to fix TransformOperation's Animate implementation [1] anyway.
> > > When that time comes, we can fix is_matched_oparation() in the same way I
> > > think.
> > 
> > Oops, I meant bug 1391145.
> 
> Ok, I know what you were saying now. So, with introducing more
> TransformOperation types bug 1391145, we may cause more unnecessary cases to
> fall into mismatched transform lists case. Though this won't regress the
> current behavior, but more like we gain some performance (in matched but
> with non-invertible matrix) in this bug and lose some more (between matched
> primitives) in bug 1391145.
> 
> However, with or without my patch in this bug, we'll have to face the issue
> anyway once we land bug 1391145, right? So, I think the issue should not
> block us from landing this bug.

Yep, right.  What I am concerned/afraid is that this fix might introduce
more complexity in bug 1391145.  As far as I know, Manish had a clear idea to solve bug 1391145 with some techniques in Rust that I don't really understand.  So I am just afraid, nothing blocks this bug. :)

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Servo PR, #18966

Comment 11

[Jeremy Chen ([e-mail])]

Merged: https://hg.mozilla.org/mozilla-central/rev/487c7f620b0c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I assume there's no intent to request 57 uplift on this. Feel free to set the status back to affected and request approval if you feel strongly otherwise.