Use FastTransform almost everywhere

[mrobinson]

This helps to avoid the cost of expensive matrix math and allows us to
more liberally call things like inverse() on our transformations,
since the worst case is that we will be retrieving the already-cached
inverted matrix. This decreases the amount of time that
FrameBuilder::build takes in my local CPU profiles.

---

This change is 

[mrobinson]

Gecko try job for this change is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f8bc89e36afa986e5f3ea8011caeec007b66cc9

The build failures seem to be unrelated infrastructure issues.

[kvark]

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

webrender/src/clip_scroll_node.rs, line 183 at r1 (raw file):

        let source_perspective = source_perspective.map_or_else(
            LayoutTransformOrOffset::identity,
            |perspective| LayoutTransformOrOffset::new_transform(perspective)

nit: could just be LayoutTransformOrOffset::new_transform

---

webrender/src/prim_store.rs, line 1475 at r1 (raw file):

                    local_clip_rect
                } else {
                    let clip_transform = LayerToWorldTransformOrOffset::new_transform(

nit: it could also just implement From<Transform> and From<Vector> instead of constructors

---

webrender/src/util.rs, line 157 at r1 (raw file):

pub fn calculate_screen_bounding_rect(
    transform: &TransformOrOffset<LayerPixel, WorldPixel>,

nit: use the new alias here

---

webrender/src/util.rs, line 347 at r1 (raw file):

/// when possible when dealing with non-perspective axis-aligned transformations.
#[derive(Debug, Clone, Copy)]
pub enum TransformOrOffset<Src, D> {

we should be consistent in naming of those generic arguments

---

webrender/src/util.rs, line 347 at r1 (raw file):

/// when possible when dealing with non-perspective axis-aligned transformations.
#[derive(Debug, Clone, Copy)]
pub enum TransformOrOffset<Src, D> {

I don't think name like XOrY is elegant. It exposes the internal detail. What is important here is that this is a transform, it quacks like one, maybe not exactly a Matrix4, but can be converted into it. So maybe FastTransform? More opinions are welcome :)

---

webrender/src/util.rs, line 349 at r1 (raw file):

pub enum TransformOrOffset<Src, D> {
    /// A simple offset, which can be used without doing any matrix math.
    Offset(TypedVector2D<f32, Src>),

I think this should use the destination space - makes it more convenient for the using code

---

webrender/src/util.rs, line 358 at r1 (raw file):

    },

    //Uninvertible,

is this comment needed?

---

webrender/src/util.rs, line 371 at r1 (raw file):

    #[inline(always)]
    pub fn new_transform(transform: TypedTransform3D<f32, Src, D>) -> Self {

doesn't look to be consistent with with_offset constructor

---

webrender/src/util.rs, line 380 at r1 (raw file):

    }

    pub fn to_transform(&self) -> TypedTransform3D<f32, Src, D> {

could probably just implement Into<>

---

webrender/src/util.rs, line 391 at r1 (raw file):

        match *self {
            TransformOrOffset::Offset(..) => true,
            TransformOrOffset::Transform { inverse, .. } => inverse.is_some(),

ref inverse should avoid copies

---

webrender/src/util.rs, line 402 at r1 (raw file):

        match (self, other) {
            (&TransformOrOffset::Offset(ref offset), &TransformOrOffset::Offset(ref other_offset)) => {
                let offset = TypedVector2D::from_untyped(&offset.to_untyped());

nit: could also do offset * TypedScale::<_, NewSrc, D>::new(1.0)

---

webrender/src/util.rs, line 407 at r1 (raw file):

            _ => {
                let new_transform = self.to_transform().pre_mul(&other.to_transform());
                TransformOrOffset::new_transform(new_transform)

is it desirable to re-compute the inverse during each step of transformations like this one?

---

webrender/src/util.rs, line 417 at r1 (raw file):

            &TransformOrOffset::Offset(ref offset) =>
                return TransformOrOffset::Offset(*offset + *other_offset),
            &TransformOrOffset::Transform { transform, .. } =>

similarly, we probably want ref transform here and below

---

webrender/src/util.rs, line 442 at r1 (raw file):

    pub fn is_backface_visible(&self) -> bool {
        match *self {
            TransformOrOffset::Offset(..) => true,

I think this should be false

---

webrender/src/util.rs, line 483 at r1 (raw file):

                Some(inverse.transform_rect(&rect)),
            TransformOrOffset::Transform { transform, is_2d: false, .. } =>
                Some(transform.inverse_rect_footprint(rect)),

not required for this PR, but I think inverse_rect_footprint should return an Option itself

---

Comments from Reviewable

[mrobinson]

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

webrender/src/clip_scroll_node.rs, line 183 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: could just be LayoutTransformOrOffset::new_transform

I think LayoutTransformOrOffset::identity is a little more efficient here.

---

webrender/src/prim_store.rs, line 1475 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: it could also just implement From<Transform> and From<Vector> instead of constructors

Good idea!

---

webrender/src/util.rs, line 157 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: use the new alias here

Nice catch.

---

webrender/src/util.rs, line 347 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we should be consistent in naming of those generic arguments

Also here. I had planned to fix this, but it got lost during the day.

---

webrender/src/util.rs, line 347 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I don't think name like XOrY is elegant. It exposes the internal detail. What is important here is that this is a transform, it quacks like one, maybe not exactly a Matrix4, but can be converted into it. So maybe FastTransform? More opinions are welcome :)

I'm happy to rename this to FastTransform.

---

webrender/src/util.rs, line 349 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think this should use the destination space - makes it more convenient for the using code

When it's an offset the Src and Dst are the same unit. I'm not sure how to express this in the type system. I tested changing this to Dst and it seems that it complicates a lot of callsites. Choosing one or the other will have convenience tradeoffs. Using Src keeps almost all of the to_untyped``/ from_untypedstuff inside the struct implementation, which I think is a good indication thatSrc` is the correct choice here.

---

webrender/src/util.rs, line 358 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is this comment needed?

This was left over. I'll remove it.

---

webrender/src/util.rs, line 371 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

doesn't look to be consistent with with_offset constructor

Fixed.

---

webrender/src/util.rs, line 380 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could probably just implement Into<>

Done.

---

webrender/src/util.rs, line 391 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

ref inverse should avoid copies

Done.

---

webrender/src/util.rs, line 402 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: could also do offset * TypedScale::<_, NewSrc, D>::new(1.0)

It's hard to say which version is more readable, but I think it's a bit easier to read without the angle brackets.

---

webrender/src/util.rs, line 407 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is it desirable to re-compute the inverse during each step of transformations like this one?

It's not necessary and we could specialize this even further, but I wanted to avoid premature optimization here.

---

webrender/src/util.rs, line 417 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

similarly, we probably want ref transform here and below

Done.

---

webrender/src/util.rs, line 442 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think this should be false

Thanks! Fixed.

---

webrender/src/util.rs, line 483 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

not required for this PR, but I think inverse_rect_footprint should return an Option itself

Okay. That's a good change for the future.

---

Comments from Reviewable

[mrobinson]

@kvark Thanks for the excellent review. I think I have answered almost all of your concerns, but let me know if I missed anything.

[kvark]

Thanks for addressing my concerns! We are almost there :)

---

Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

webrender/src/clip_scroll_node.rs, line 183 at r1 (raw file):

Previously, mrobinson (Martin Robinson) wrote…

I think LayoutTransformOrOffset::identity is a little more efficient here.

this comment was about the line you changed to |perspective| perspective.into(), which btw could also be written as From::from

---

webrender/src/util.rs, line 371 at r1 (raw file):

Previously, mrobinson (Martin Robinson) wrote…

Fixed.

do we still need the constructors though?..

---

webrender/src/util.rs, line 402 at r1 (raw file):

Previously, mrobinson (Martin Robinson) wrote…

It's hard to say which version is more readable, but I think it's a bit easier to read without the angle brackets.

Sure, I don't hold strong feelings about this. Just seeing untyped makes me cringe :)

---

webrender/src/util.rs, line 115 at r2 (raw file):

        }

        self.m43 < NEARLY_ZERO

I think this should come with abs()

---

Comments from Reviewable

[mrobinson]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

webrender/src/util.rs, line 371 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

do we still need the constructors though?..

The constructors are still used in a couple places to avoid having to use explicitly specify the types in order to satisfy the type inference. I think it's better to be explicit in those cases rather than rely on the inference + writing the type.

---

webrender/src/util.rs, line 115 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think this should come with abs()

Okay. Fixed.

---

Comments from Reviewable

[kvark]

@bors-servo r+

[bors-servo]

📌 Commit 8c3bf9c has been approved by kvark

[bors-servo]

⌛ Testing commit 8c3bf9c with merge b2acbc9...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: kvark
Pushing b2acbc9 to master...