Closed Bug 1417376 Opened 2 years ago Closed 2 years ago

Convert nsPoint parameters to passing by const reference instead values

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

Details

Attachments

(3 files)

Some of the functions under layout are passing nsPoint by values with modifying them. We could convert them to passing by const references to save some copy constructor calls. 

https://dxr.mozilla.org/mozilla-central/search?q=%22nsPoint+aPoint%22+path%3Alayout&redirect=false
Comment on attachment 8928823 [details]
Bug 1417376 Part 3 - Pass nsPoint parameters by const references instead of values.

https://reviewboard.mozilla.org/r/200088/#review205254


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: layout/generic/BRFrame.cpp:35
(Diff revision 1)
>  public:
>    NS_DECL_FRAMEARENA_HELPERS(BRFrame)
>  
>    friend nsIFrame* ::NS_NewBRFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
>  
> -  virtual ContentOffsets CalcContentOffsetsFromFramePoint(nsPoint aPoint) override;
> +  virtual ContentOffsets CalcContentOffsetsFromFramePoint(const nsPoint& aPoint) override;

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8928823 [details]
Bug 1417376 Part 3 - Pass nsPoint parameters by const references instead of values.

https://reviewboard.mozilla.org/r/200088/#review205254

> Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

Fixed.
Attachment #8928821 - Flags: review?(dholbert) → review?(mats)
Attachment #8928822 - Flags: review?(dholbert) → review?(mats)
Attachment #8928823 - Flags: review?(dholbert) → review?(mats)
Comment on attachment 8928821 [details]
Bug 1417376 Part 1 - Change nsPoint parameter to pass by value for DoAutoScroll().

https://reviewboard.mozilla.org/r/200084/#review206166
Attachment #8928821 - Flags: review?(mats) → review+
I'm not sure if "const nsPoint&" is better than "nsPoint".  It might
actually be slower.  I would guess that nsPoint can be passed in
a register on 64-bit platforms since it's just two 32-bit integers.
The advantage is that the called function can read the value directly
rather than indirectly through a pointer.

The functions in part 2 / 3 probably aren't that performance critical
that it matters much, but it would be nice if we pick one or the other
and stick with it for consistency.  I'd probably pick nsPoint since I
suspect it's slightly faster in general (a few posts on stackoverflow
agrees).

Any other arguments for choosing one or the other?
(In reply to Mats Palmgren (:mats) from comment #8)
> I'm not sure if "const nsPoint&" is better than "nsPoint".  It might
> actually be slower.  I would guess that nsPoint can be passed in
> a register on 64-bit platforms since it's just two 32-bit integers.
> The advantage is that the called function can read the value directly
> rather than indirectly through a pointer.

I think your argument is valid. I wrote a small example to compare the assembly between passing by const-reference and by value. The compiler is smart enough not to call the copy constructor when passing Point struct by value and the called function can read the value without through a pointer.
https://godbolt.org/g/gy73Qr

> The functions in part 2 / 3 probably aren't that performance critical
> that it matters much, but it would be nice if we pick one or the other
> and stick with it for consistency.  I'd probably pick nsPoint since I
> suspect it's slightly faster in general (a few posts on stackoverflow
> agrees).

I think we should pick nsPoint (or maybe 'const nsPoint' if it's not modified). I'll change part 2 to use nsPoint, and discard part 3.

> Any other arguments for choosing one or the other?

I don't have other arguments other than eliminating copy constructor calls.
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #9)
> I think your argument is valid. I wrote a small example to compare the
> assembly between passing by const-reference and by value. The compiler is
> smart enough not to call the copy constructor when passing Point struct by
> value and the called function can read the value without through a pointer.
> https://godbolt.org/g/gy73Qr

This example is wrong because it doesn't contain copy constructor.

Here's the example that has a copy constructor under optimization. Both functions that passing Point by const-reference and by value look the same. So I guess it probably doesn't matter which one we use.

https://godbolt.org/g/PzfhPU
Interesting.  Fwiw, I moved the definitions of f1/f2 to a separate file
and just declared them "extern" in the main file and then there is
definitely a copy made on the stack.  So I think you're right that
"const nsPoint&" would be faster in general (although when the called
function is inlined it might be optimized away).
Comment on attachment 8928822 [details]
Bug 1417376 Part 2 - Pass nsPoint parameters by const references instead of references.

https://reviewboard.mozilla.org/r/200086/#review206200

::: dom/base/Selection.h:179
(Diff revision 1)
>      bool aSlowCheck);
>  
>    NS_IMETHOD   Repaint(nsPresContext* aPresContext);
>  
>    // Note: StartAutoScrollTimer might destroy arbitrary frames etc.
>    nsresult     StartAutoScrollTimer(nsIFrame *aFrame,

while we're here - make this "nsIFrame* aFrame" please?

::: layout/generic/nsFrameSelection.h:611
(Diff revision 1)
>    nsresult ConstrainFrameAndPointToAnchorSubtree(nsIFrame *aFrame,
> -                                                 nsPoint& aPoint,
> +                                                 const nsPoint& aPoint,
>                                                   nsIFrame **aRetFrame,

ditto, nsIFrame* and nsIFrame**

::: layout/generic/nsFrameSelection.cpp:452
(Diff revision 1)
>  nsFrameSelection::ConstrainFrameAndPointToAnchorSubtree(nsIFrame  *aFrame,
> -                                                        nsPoint&   aPoint,
> +                                                        const nsPoint& aPoint,
>                                                          nsIFrame **aRetFrame,

and here too
Attachment #8928822 - Flags: review?(mats) → review+
Comment on attachment 8928823 [details]
Bug 1417376 Part 3 - Pass nsPoint parameters by const references instead of values.

https://reviewboard.mozilla.org/r/200088/#review206202

::: layout/generic/nsFrame.cpp:457
(Diff revision 2)
>  nsIFrame::FindCloserFrameForSelection(
> -                                 nsPoint aPoint,
> +  const nsPoint& aPoint,
> -                                 nsIFrame::FrameWithDistance* aCurrentBestFrame)
> +  nsIFrame::FrameWithDistance* aCurrentBestFrame)

nit: maybe this can probably be written as:
nsIFrame::FindCloserFrameForSelection(const nsPoint&     aPoint, 
                                      FrameWithDistance* aCurrentBestFrame)

(i.e. the nsIFrame:: prefix on FrameWithDistance seems unnecessary here)

::: layout/generic/nsFrameSelection.h:323
(Diff revision 2)
>    nsresult StartAutoScrollTimer(nsIFrame *aFrame,
> -                                nsPoint aPoint,
> +                                const nsPoint& aPoint,

nit: nsIFrame* while we're here

::: layout/generic/nsFrameSelection.cpp:1339
(Diff revision 2)
>    HandleClick(offsets.content, offsets.offset, offsets.offset,
>                true, false, offsets.associate);
>  }
>  
>  nsresult
>  nsFrameSelection::StartAutoScrollTimer(nsIFrame *aFrame,

ditto

::: layout/svg/SVGTextFrame.cpp:3426
(Diff revision 2)
>  SVGTextFrame::FindCloserFrameForSelection(
> -                                 nsPoint aPoint,
> +  const nsPoint& aPoint,
> -                                 nsIFrame::FrameWithDistance* aCurrentBestFrame)
> +  nsIFrame::FrameWithDistance* aCurrentBestFrame)

nit: probably don't need the nsIFrame:: prefix here either
Attachment #8928823 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #11)
> Interesting.  Fwiw, I moved the definitions of f1/f2 to a separate file
> and just declared them "extern" in the main file and then there is
> definitely a copy made on the stack.  So I think you're right that
> "const nsPoint&" would be faster in general (although when the called
> function is inlined it might be optimized away).

It's good to know there are still differences if the definitions are in a separate file!

The latest patches addressed the review comments in comment 12 and comment 13.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17c1e67055b1
Part 1 - Change nsPoint parameter to pass by value for DoAutoScroll(). r=mats
https://hg.mozilla.org/integration/autoland/rev/cf383c0b3d5f
Part 2 - Pass nsPoint parameters by const references instead of references. r=mats
https://hg.mozilla.org/integration/autoland/rev/25dfc9b677cb
Part 3 - Pass nsPoint parameters by const references instead of values. r=mats
https://hg.mozilla.org/mozilla-central/rev/17c1e67055b1
https://hg.mozilla.org/mozilla-central/rev/cf383c0b3d5f
https://hg.mozilla.org/mozilla-central/rev/25dfc9b677cb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.