RVO is expected, but not happening

97 views Asked by At

I have the following rudimentary implementation of a connection pool. At a very high level, it provides a safe way for a client to get an open connection from a pool, or open a new one if all the connections are currently in use. The Connection struct is the low level implementation of a connection. The ClientConnection struct is a wrapper around the Connection so that the client has only limited access to the Connection object as well as providing a safe way for the Connection object to be returned to the pool using RAII. When calling Pool::getNewConnection(), I expect NRVO to occur, but it does not and I cannot for the life of me figure out why. I expect only a single call to the destructor, since I create the ClientConnection in main() and then it gets destroyed. However, what happens is an instance is created in Pool::getNewConnection(), then moved into the object's memory in main(), thus 2 destructors are called.

Built using g++ -std=c++17 -O0 -g -o main main2.cpp and g++ -std=c++17 -O3 -g -o main main2.cpp

Output is:

$ ./main 
Opening connection
Pool::getNewConnection() -- Address of newClientConnection: 0x7ffe3d29c290
ClientConnection(ClientConnection&&) -- old: 0x7ffe3d29c290 new: 0x7ffe3d29c320
ClientConnection::~ClientConnection() -- Returning this connection back to pool: 0x7ffe3d29c290
Pool::returnConnection() -- Returned connection: 0x7ffe3d29c290
Writing
ClientConnection::~ClientConnection() -- Returning this connection back to pool: 0x7ffe3d29c320
Pool::returnConnection() -- Returned connection: 0x7ffe3d29c320
#include <iostream>
#include <memory>
#include <vector>

struct Connection {
  bool open(std::string /* ip */, std::uint16_t /* port */) {
    std::cout << "Opening connection\n";
    return true;
  }
  bool close() {
    std::cout << "Closing connection\n";
    return true;
  }
  bool write(const char*) { return true; }
  bool read(const char*) { return true; }
};

struct ClientConnection;

struct Pool {
  ClientConnection getNewConnection();
  void returnConnection(ClientConnection& clientConnection);

private:
  friend class ClientConnection;

  std::vector<std::unique_ptr<Connection>> m_connections;
};

struct ClientConnection {
  ClientConnection()                        = delete;
  ClientConnection(const ClientConnection&) = delete;
  ClientConnection(ClientConnection&& rhs) noexcept : m_pool(rhs.m_pool), m_connection(std::move(rhs.m_connection)) {
    std::cout << "ClientConnection(ClientConnection&&) -- old: " << std::addressof(rhs) << " new: " << this
              << std::endl;
  }

  ClientConnection& operator=(const ClientConnection&) = delete;
  ClientConnection& operator=(ClientConnection&&) = delete;
  ~ClientConnection() noexcept {
    std::cout << "ClientConnection::~ClientConnection() -- Returning this connection back to pool: " << this
              << std::endl;
    m_pool.returnConnection(*this);
  }

  bool write(const char*) {
    std::cout << "Writing\n";
    if (!m_connection->write(nullptr)) {
      m_connectionValid = false;
    }
    return m_connectionValid;
  }

  bool read(const char*) {
    if (!m_connection->read(nullptr)) {
      m_connectionValid = false;
    }
    return m_connectionValid;
  }

private:
  friend class Pool;

  ClientConnection(Pool& pool, std::unique_ptr<Connection> connection)
      : m_pool(pool), m_connection(std::move(connection)) {}

private:
  bool m_connectionValid = true;
  Pool& m_pool;
  std::unique_ptr<Connection> m_connection;
};

ClientConnection Pool::getNewConnection() {
  if (m_connections.empty()) {
    auto conn = std::make_unique<Connection>();
    conn->open("", 0);
    auto newClientConnection = ClientConnection(*this, std::move(conn));
    std::cout << "Pool::getNewConnection() -- Address of newClientConnection: " << std::addressof(newClientConnection)
              << std::endl;
    return newClientConnection;
  } else {
    auto newClientConnection = ClientConnection(*this, std::move(m_connections.back()));
    m_connections.erase(std::prev(m_connections.end(), 1));
    return newClientConnection;
  }
}

void Pool::returnConnection(ClientConnection& clientConnection) {
  if (clientConnection.m_connectionValid) {
    m_connections.push_back(std::move(clientConnection.m_connection));
    std::cout << "Pool::returnConnection() -- Returned connection: " << std::addressof(clientConnection) << std::endl;
  } else {
    clientConnection.m_connection->close();
  }
}

int main() {
  Pool p;
  auto conn = p.getNewConnection();
  conn.write(nullptr);
  return 0;
}
1

There are 1 answers

0
Jarod42 On

NRVO is not guaranteed. Compiler might miss the opportunity to use it.

Alternative to ensure copy-elision is to use alternative form which are guaranteed.

Something like the following:

ClientConnection Pool::getNewConnection()
{
    if (m_connections.empty()) {
        auto conn = std::make_unique<Connection>();
        conn->open("", 0);
        // Skip (problematic) debug display which introduces name for ClientConnection 
        return {*this, std::move(conn)};
    } else {
        struct OnScopeEnd // You might have equivalent util class
        {
            std::vector<std::unique_ptr<Connection>>& m_connections;
            ~OnScopeEnd() { m_connections.pop_back(); }
        } onScopeEnd{m_connections};
        return {*this, std::move(m_connections.back())};
    }
}