CBlockLocator: performance-move-const-arg Clang tidy fixup #25963

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:performance-move-const-arg-fixups changing 2 files +2 −2
  1. jonatack commented at 11:12 AM on August 31, 2022: contributor

    Fix Clang-tidy CI errors on master. See https://cirrus-ci.com/task/4806752200818688?logs=ci#L4696 for an example.

  2. jonatack marked this as ready for review on Aug 31, 2022
  3. jonatack commented at 11:31 AM on August 31, 2022: contributor

    The Clang tidy CI passed, so opening this up for review.

  4. sipa commented at 11:47 AM on August 31, 2022: member

    The change on line 52 makes sense, but the one on 316 seems like a step backwards?

  5. jonatack force-pushed on Aug 31, 2022
  6. jonatack commented at 12:04 PM on August 31, 2022: contributor

    The change on line 52 makes sense, but the one on 316 seems like a step backwards?

    Hm, RVO wouldn't be affected by the move of an argument (rather than of the object itself)?

    Repushed without that change to see if the tidy job remains appeased.

  7. sipa commented at 12:09 PM on August 31, 2022: member

    Hm, RVO wouldn't be affected by the move of an argument (rather than of the object itself)?

    I don't see how RVO could apply here. It's an argument to a constructor. The fact that that constructor is invoked in a return statement doesn't matter.

  8. jonatack renamed this:
    CBlockLocator: performance-move-const-arg Clang tidy fixups
    CBlockLocator: performance-move-const-arg Clang tidy fixup
    on Aug 31, 2022
  9. jonatack commented at 12:27 PM on August 31, 2022: contributor 
    src/headerssync.cpp:316:26: error: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg,-warnings-as-errors]
    return CBlockLocator{std::move(locator)};
                         ^~~~~~~~~~       ~
  10. jonatack force-pushed on Aug 31, 2022
  11. sipa commented at 12:45 PM on August 31, 2022: member

    @jonatack Oh! The issue is just that CBlockLocator has no constructor that accepts an rvalue uint256. The std::move there has no effect, but the correct solution is adding an rvalue-accepting constructor, not getting rid of the std::move.

  12. jonatack commented at 12:53 PM on August 31, 2022: contributor

    @jonatack Oh! The issue is just that CBlockLocator has no constructor that accepts an rvalue uint256. The std::move there has no effect, but the correct solution is adding an rvalue-accepting constructor, not getting rid of the std::move.

    Ah interesting! Learning.

  13. vasild commented at 12:54 PM on August 31, 2022: contributor

    ACK af7a40c1830c16cb0005a4fb121f631e01be9f80 (the previous push with the two changes)

    Both warnings are legit because no move will happen in either case.

    But consider this, like @sipa suggested above, would avoid copying:

    <details> <summary>diff</summary>

    diff --git i/src/chain.cpp w/src/chain.cpp
index 19c35b5012..66a0830394 100644
--- i/src/chain.cpp
+++ w/src/chain.cpp
@@ -46,13 +46,13 @@ std::vector<uint256> LocatorEntries(const CBlockIndex* index)
     }
     return have;
 }

 CBlockLocator GetLocator(const CBlockIndex* index)
 {
-    return CBlockLocator{std::move(LocatorEntries(index))};
+    return CBlockLocator{LocatorEntries(index)};
 }

 CBlockLocator CChain::GetLocator() const
 {
     return ::GetLocator(Tip());
 }
diff --git i/src/primitives/block.h w/src/primitives/block.h
index 76aba6c899..8c51d280b8 100644
--- i/src/primitives/block.h
+++ w/src/primitives/block.h
@@ -120,13 +120,13 @@ public:
 struct CBlockLocator
 {
     std::vector<uint256> vHave;

     CBlockLocator() {}

-    explicit CBlockLocator(const std::vector<uint256>& vHaveIn) : vHave(vHaveIn) {}
+    explicit CBlockLocator(std::vector<uint256>&& vHaveIn) : vHave(std::move(vHaveIn)) {}

     SERIALIZE_METHODS(CBlockLocator, obj)
     {
         int nVersion = s.GetVersion();
         if (!(s.GetType() & SER_GETHASH))
             READWRITE(nVersion);

    </details>

  14. MarcoFalke commented at 12:58 PM on August 31, 2022: member

    Using && would force all call sites to use std::move. Thus, it makes it impossible to use on const&.

    You can simply change the constructor to allow anything:

    explicit CBlockLocator(std::vector<uint256> have) : vHave{std::move(have)} {}
  15. sipa commented at 1:05 PM on August 31, 2022: member

    @MarcoFalke I think all call sites already pass in rvalues. And an rvalue argument is still more efficient than a by-value one, as it avoids constructing the temporary vector entirely (not just avoiding allocating its storage buffer).

  16. CBlockLocator: performance-move-const-arg Clang tidy fixups
    Co-authored-by: "Pieter Wuille <pieter@wuille.net>"
Co-authored-by: "Vasil Dimov <vd@FreeBSD.org>"
Co-authored-by: "MarcoFalke <falke.marco@gmail.com>"
    6b24dfe24d
  17. jonatack force-pushed on Aug 31, 2022
  18. MarcoFalke commented at 1:13 PM on August 31, 2022: member

    an rvalue argument is still more efficient than a by-value one

    Correct. Personally, I think it is fine to have an overhead equivalent to constructing an empty vector. When there is a new call-site with a const reference in the future, the constructor will need to be changed anyway. Either a single one that accepts a pure value, or a new one that accepts a const& (alongside the && constructor), which is what the stdlib is usually doing.

    But anything should be fine here.

    review ACK 6b24dfe24d2ed50ea0b06ce1919db3dc0e871a03

  19. MarcoFalke added the label Refactoring on Aug 31, 2022
  20. MarcoFalke added this to the milestone 24.0 on Aug 31, 2022
  21. vasild commented at 1:28 PM on August 31, 2022: contributor

    With a by-value argument there is construct+move+move, with an rval argument, there is construct+move:

    <details> <summary>rv.cc</summary>

    #include <iostream>

/* The rule of five. */
class R5
{
public:
    R5(int x, double y)
    {
        x_ = x;
        y_ = y;
        std::cout << "R5::R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
    }

    ~R5()
    {
        std::cout << "R5::~R5() this=" << this << " x=" << x_ << ", y=" << y_ << std::endl;
    }

    R5(const R5& other)
    {
        std::cout << "R5::R5(const R5&) this=" << this << " other=" << &other << " x=" << other.x_
                  << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
    }

    R5(R5&& other)
    {
        std::cout << "R5::R5(R5&&) this=" << this << " other=" << &other << " x=" << other.x_
                  << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        other.x_ = 0;
        other.y_ = 0;
    }

    R5& operator=(const R5& other)
    {
        std::cout << "R5::operator=(const R5&) this=" << this << " x=" << x_ << ", y=" << y_
                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        return *this;
    }

    R5& operator=(R5&& other)
    {
        std::cout << "R5::operator=(R5&&) this=" << this << " x=" << x_ << ", y=" << y_
                  << " other=" << &other << " x=" << other.x_ << ", y=" << other.y_ << std::endl;
        x_ = other.x_;
        y_ = other.y_;
        other.x_ = 0;
        other.y_ = 0;
        return *this;
    }

    std::ostream& operator<<(std::ostream& os) { return os << "(x=" << x_ << ", y=" << y_ << ")"; }

    int x_;
    double y_;
};

class CBlockLocator
{
public:
#if 0
    explicit CBlockLocator(R5 r5) : m_r5{std::move(r5)} {}
#else
    explicit CBlockLocator(R5&& r5) : m_r5{std::move(r5)} {}
#endif

    R5 m_r5;
};

CBlockLocator f3()
{
    R5 r5{2, 3.4};
    return CBlockLocator{std::move(r5)};
}

int main(int, char**)
{
    f3();
    return 0;
}

    </details>

    output with R5 r5 arg:

    R5::R5() this=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe5d8 other=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5d8 x=2, y=3.4
R5::~R5() this=0x7fffffffe5d8 x=0, y=0
R5::~R5() this=0x7fffffffe5e8 x=0, y=0
R5::~R5() this=0x7fffffffe610 x=2, y=3.4

    output with R5&& r5 arg:

    R5::R5() this=0x7fffffffe5e8 x=2, y=3.4
R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5e8 x=2, y=3.4
R5::~R5() this=0x7fffffffe5e8 x=0, y=0
R5::~R5() this=0x7fffffffe610 x=2, y=3.4
  22. vasild approved
  23. vasild commented at 1:29 PM on August 31, 2022: contributor

    ACK 6b24dfe24d2ed50ea0b06ce1919db3dc0e871a03

  24. sipa commented at 1:46 PM on August 31, 2022: member

    Correct. Personally, I think it is fine to have an overhead equivalent to constructing an empty vector. When there is a new call-site with a const reference in the future, the constructor will need to be changed anyway. Either a single one that accepts a pure value, or a new one that accepts a const& (alongside the && constructor), which is what the stdlib is usually doing.

    Yes, to be clear: none of the changes in this PR are relevant for performance. CBlockLocators are constructed sufficiently rarely that shaving off (maybe) a microsecond here and there isn't going to be measurable. But if we're already talking about changing the code anyway, it's good to follow best practices.

  25. MarcoFalke merged this on Aug 31, 2022
  26. MarcoFalke closed this on Aug 31, 2022
  27. jonatack deleted the branch on Aug 31, 2022
  28. MarcoFalke commented at 2:02 PM on August 31, 2022: member

    Going to merge now to unbreak CI. If there is anything else to be done here, it can be done in a follow-up.

  29. sidhujag referenced this in commit ddbf7e76be on Sep 1, 2022
  30. jonatack commented at 10:05 AM on September 2, 2022: contributor

    With a by-value argument there is construct+move+move, with an rval argument, there is construct+move: rv.cc

    output with `R5 r5` arg:

    R5::R5() this=0x7fffffffe5e8 x=2, y=3.4 R5::R5(R5&&) this=0x7fffffffe5d8 other=0x7fffffffe5e8 x=2, y=3.4 R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5d8 x=2, y=3.4 R5::~R5() this=0x7fffffffe5d8 x=0, y=0 R5::~R5() this=0x7fffffffe5e8 x=0, y=0 R5::~R5() this=0x7fffffffe610 x=2, y=3.4

    
output with `R5&& r5` arg:

    R5::R5() this=0x7fffffffe5e8 x=2, y=3.4 R5::R5(R5&&) this=0x7fffffffe610 other=0x7fffffffe5e8 x=2, y=3.4 R5::~R5() this=0x7fffffffe5e8 x=0, y=0 R5::~R5() this=0x7fffffffe610 x=2, y=3.4

    [@vasild](/bitcoin-bitcoin/contributor/vasild/) nice! Reproduced this locally for fun.

    ₿ g++ by-val-versus-rval.cpp && ./a.out 
R5::R5() this=0x7ffe5de81ea0 x=2, y=3.4
R5::R5(R5&&) this=0x7ffe5de81eb0 other=0x7ffe5de81ea0 x=2, y=3.4
R5::R5(R5&&) this=0x7ffe5de81ef0 other=0x7ffe5de81eb0 x=2, y=3.4
R5::~R5() this=0x7ffe5de81eb0 x=0, y=0
R5::~R5() this=0x7ffe5de81ea0 x=0, y=0
R5::~R5() this=0x7ffe5de81ef0 x=2, y=3.4

₿ g++ by-val-versus-rval.cpp && ./a.out 
R5::R5() this=0x7ffea6450b90 x=2, y=3.4
R5::R5(R5&&) this=0x7ffea6450bd0 other=0x7ffea6450b90 x=2, y=3.4
R5::~R5() this=0x7ffea6450b90 x=0, y=0
R5::~R5() this=0x7ffea6450bd0 x=2, y=3.4
  31. bitcoin locked this on Sep 2, 2023
Contributors
jonatacksipavasildMarcoFalke
Labels
Refactoring

Milestone
24.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me