Closed Bug 1449145 Opened 2 years ago Closed 2 years ago

Improve by modernising code in gfx/layers/apz

Categories

(Core :: Panning and Zooming, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: andi, Assigned: andi)

Details

Attachments

(1 file)

Because of this issue has been reported for our static-analysis bot, https://github.com/mozilla-releng/services/issues/999#issuecomment-375932962, I ran our clang based analysis on the entire gfx/layers/apz. This patch includes common sense fixes like:

1. Use default instead of empty ctor or dtor
2. Avoid copy value use instead cont&
3. Avoid pass by value rather pass by T&
4. Avoid using for loops and replace with range based iterators.
Attachment #8962676 - Flags: review?(botond)
Comment on attachment 8962676 [details]
Bug 1449145 - Improve by modernising code in gfx/layers/apz.

https://reviewboard.mozilla.org/r/231540/#review237082

Some drive-by comments since Botond is off today.

Also I have a question about the "= delete" usage - is the generated binary going to be any different? Because unless it gives some perf benefit I prefer to actually have the empty constructor/destructor in the .cpp file. That way if we need to instrument those sites (with logging or whatever, which happens suprisingly frequently) we don't have to touch the header file and incur an expensive recompilation cost.

::: gfx/layers/apz/src/APZCTreeManager.cpp
(Diff revision 1)
> -    Matrix4x4 asyncUntransform = (asyncTransform * overscroll).Inverse().ToUnknownMatrix();
> -    Matrix4x4 contentTransform = aScrollableContentTransform;
> -    Matrix4x4 contentUntransform = contentTransform.Inverse();
>  
>      AsyncTransformComponentMatrix asyncCompensation =
>          ViewAs<AsyncTransformComponentMatrix>(
> -            contentTransform
> -          * asyncUntransform
> -          * contentUntransform);
> +            aScrollableContentTransform
> +          * (asyncTransform * overscroll).Inverse().ToUnknownMatrix()
> +          * aScrollableContentTransform.Inverse());

Undo this change. You're dropping useful variable names.

::: gfx/layers/apz/src/CheckerboardEvent.cpp:162
(Diff revision 1)
>    if (!mRecordTrace) {
>      return;
>    }
>    MonitorAutoLock lock(mRendertraceLock);
>    std::vector<PropertyValue> history;
> -  for (size_t i = 0; i < sRendertracePropertyCount; i++) {
> +  for (PropertyBuffer& BufferedProperty : mBufferedProperties) {

local vars should start with a lowercase letter
Component: Graphics: Layers → Panning and Zooming
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Comment on attachment 8962676 [details]
> Bug 1449145 - Improve by modernising code in gfx/layers/apz.
> 
> https://reviewboard.mozilla.org/r/231540/#review237082
> 
> Some drive-by comments since Botond is off today.
> 
> Also I have a question about the "= delete" usage - is the generated binary
> going to be any different? Because unless it gives some perf benefit I
> prefer to actually have the empty constructor/destructor in the .cpp file.
> That way if we need to instrument those sites (with logging or whatever,
> which happens suprisingly frequently) we don't have to touch the header file
> and incur an expensive recompilation cost.
> 
The = default checker has been implemented since it can have more opportunities of optimisation. 
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.html

> ::: gfx/layers/apz/src/APZCTreeManager.cpp
> (Diff revision 1)
> > -    Matrix4x4 asyncUntransform = (asyncTransform * overscroll).Inverse().ToUnknownMatrix();
> > -    Matrix4x4 contentTransform = aScrollableContentTransform;
> > -    Matrix4x4 contentUntransform = contentTransform.Inverse();
> >  
> >      AsyncTransformComponentMatrix asyncCompensation =
> >          ViewAs<AsyncTransformComponentMatrix>(
> > -            contentTransform
> > -          * asyncUntransform
> > -          * contentUntransform);
> > +            aScrollableContentTransform
> > +          * (asyncTransform * overscroll).Inverse().ToUnknownMatrix()
> > +          * aScrollableContentTransform.Inverse());
> 
> Undo this change. You're dropping useful variable names.
Agree but we are copying Matrix4x4, maybe if we would use at least a constT& for contentTransform?
> 
> ::: gfx/layers/apz/src/CheckerboardEvent.cpp:162
> (Diff revision 1)
> >    if (!mRecordTrace) {
> >      return;
> >    }
> >    MonitorAutoLock lock(mRendertraceLock);
> >    std::vector<PropertyValue> history;
> > -  for (size_t i = 0; i < sRendertracePropertyCount; i++) {
> > +  for (PropertyBuffer& BufferedProperty : mBufferedProperties) {
> 
> local vars should start with a lowercase letter
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #3)
> The = default checker has been implemented since it can have more
> opportunities of optimisation. 
> https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-equals-default.
> html

Ah, according to this you can use the = default at the implementation site. You could do something like:

// Foo.h

struct Foo {
  Foo();
  ~Foo();
};

// Foo.cpp

Foo::Foo() = default;
Foo::~Foo() = default;

which would allow the compiler optimization and also make it easy to instrument without touching the .h file.

> > Undo this change. You're dropping useful variable names.
> Agree but we are copying Matrix4x4, maybe if we would use at least a constT&
> for contentTransform?

Yeah const-ref for contentTransform should be ok.
can you review this? Also according to our coding style we prefer = default.
Comment on attachment 8962676 [details]
Bug 1449145 - Improve by modernising code in gfx/layers/apz.

https://reviewboard.mozilla.org/r/231540/#review237636

Thanks for working on this!

::: gfx/layers/apz/src/APZCTreeManager.h:528
(Diff revision 3)
>        bool aScrollbarIsDescendant,
>        AsyncTransformComponentMatrix* aOutClipTransform);
>  
>  protected:
>    // Protected destructor, to discourage deletion outside of Release():
> -  virtual ~APZCTreeManager();
> +  virtual ~APZCTreeManager() = default;

I second Kats' request that, in cases where the constructor or destructor was defined out of line in a different file, to place the |= default| on the out-of-line definition instead of the declaration, so we can add code to the constructor/destructor if desired without touching the header file.
Attachment #8962676 - Flags: review?(botond)
Comment on attachment 8962676 [details]
Bug 1449145 - Improve by modernising code in gfx/layers/apz.

https://reviewboard.mozilla.org/r/231540/#review237656

Thanks!
Attachment #8962676 - Flags: review?(botond) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7db0e20d6a37
Improve by modernising code in gfx/layers/apz. r=botond
https://hg.mozilla.org/mozilla-central/rev/7db0e20d6a37
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.