Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle peer addition/removal in a right way #164

Merged
merged 3 commits into from
Jun 7, 2021
Merged

Handle peer addition/removal in a right way #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efb7e5a
qt, refactor: Use default arguments for overridden functions
hebasto Feb 22, 2021
1b66f6e
qt: Drop PeerTablePriv class
hebasto Feb 22, 2021
ecbd911
qt: Handle peer addition/removal in a right way
hebasto Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 44 additions & 52 deletions src/qt/peertablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,52 +14,11 @@
#include <QList>
#include <QTimer>

// private implementation
class PeerTablePriv
{
public:
/** Local cache of peer information */
QList<CNodeCombinedStats> cachedNodeStats;

/** Pull a full list of peers from vNodes into our cache */
void refreshPeers(interfaces::Node& node)
{
cachedNodeStats.clear();

interfaces::Node::NodesStats nodes_stats;
node.getNodesStats(nodes_stats);
cachedNodeStats.reserve(nodes_stats.size());
for (const auto& node_stats : nodes_stats)
{
CNodeCombinedStats stats;
stats.nodeStats = std::get<0>(node_stats);
stats.fNodeStateStatsAvailable = std::get<1>(node_stats);
stats.nodeStateStats = std::get<2>(node_stats);
cachedNodeStats.append(stats);
}
}

int size() const
{
return cachedNodeStats.size();
}

CNodeCombinedStats *index(int idx)
{
if (idx >= 0 && idx < cachedNodeStats.size())
return &cachedNodeStats[idx];

return nullptr;
}
};

PeerTableModel::PeerTableModel(interfaces::Node& node, QObject* parent) :
QAbstractTableModel(parent),
m_node(node),
timer(nullptr)
{
priv.reset(new PeerTablePriv());

// set up timer for auto refresh
timer = new QTimer(this);
connect(timer, &QTimer::timeout, this, &PeerTableModel::refresh);
Expand All @@ -84,23 +43,23 @@ void PeerTableModel::stopAutoRefresh()
timer->stop();
}

int PeerTableModel::rowCount(const QModelIndex &parent) const
int PeerTableModel::rowCount(const QModelIndex& parent) const
{
if (parent.isValid()) {
return 0;
}
return priv->size();
return m_peers_data.size();
}

int PeerTableModel::columnCount(const QModelIndex &parent) const
int PeerTableModel::columnCount(const QModelIndex& parent) const
{
if (parent.isValid()) {
return 0;
}
return columns.length();
}

QVariant PeerTableModel::data(const QModelIndex &index, int role) const
QVariant PeerTableModel::data(const QModelIndex& index, int role) const
{
if(!index.isValid())
return QVariant();
Expand Down Expand Up @@ -172,19 +131,52 @@ Qt::ItemFlags PeerTableModel::flags(const QModelIndex &index) const
return retval;
}

QModelIndex PeerTableModel::index(int row, int column, const QModelIndex &parent) const
QModelIndex PeerTableModel::index(int row, int column, const QModelIndex& parent) const
{
Q_UNUSED(parent);
CNodeCombinedStats *data = priv->index(row);

if (data)
return createIndex(row, column, data);
if (0 <= row && row < rowCount() && 0 <= column && column < columnCount()) {
return createIndex(row, column, const_cast<CNodeCombinedStats*>(&m_peers_data[row]));
}

return QModelIndex();
}

void PeerTableModel::refresh()
{
Q_EMIT layoutAboutToBeChanged();
priv->refreshPeers(m_node);
Q_EMIT layoutChanged();
interfaces::Node::NodesStats nodes_stats;
m_node.getNodesStats(nodes_stats);
decltype(m_peers_data) new_peers_data;
new_peers_data.reserve(nodes_stats.size());
for (const auto& node_stats : nodes_stats) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, my suggestion was not good enought, you can actually move values out of that local nodes_stats:

    for (auto&& node_stats : nodes_stats) {
        const CNodeCombinedStats stats{std::get<0>(std::move(node_stats)), 
                                       std::get<2>(std::move(node_stats)),
                                       std::get<1>(std::move(node_stats))};

(note loop variable is no longer cost reference!)

Or even with an algorithm (also moves, at least it does on local test), if one would care about "No raw loops" mantra :)

    std::transform(nodes_stats.begin(), nodes_stats.end(), std::back_inserter(new_peers_data), [](auto&& i) {
        return CNodeCombinedStats{std::get<0>(std::move(i)), std::get<2>(std::move(i)), std::get<1>(std::move(i))};
    });

Though moving std::transform seems fishy, not sure if it's "legal" to move source... But gcc 10.2.1 does that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, my suggestion was not good enought...

It seems like a kind of premature optimization, no?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, probably. Since there will be mostly on up to ~128 nodes by default, "nobody" would care if there are some extra copies I guess, and std::transform variant do look particularly "noisy" (i.e. "optimization" not worth the effort/readability cost).

const CNodeCombinedStats stats{std::get<0>(node_stats), std::get<2>(node_stats), std::get<1>(node_stats)};
new_peers_data.append(stats);
}

// Handle peer addition or removal as suggested in Qt Docs. See:
// - https://doc.qt.io/qt-5/model-view-programming.html#inserting-and-removing-rows
// - https://doc.qt.io/qt-5/model-view-programming.html#resizable-models
// We take advantage of the fact that the std::vector returned
// by interfaces::Node::getNodesStats is sorted by nodeid.
for (int i = 0; i < m_peers_data.size();) {
if (i < new_peers_data.size() && m_peers_data.at(i).nodeStats.nodeid == new_peers_data.at(i).nodeStats.nodeid) {
++i;
continue;
}
// A peer has been removed from the table.
beginRemoveRows(QModelIndex(), i, i);
m_peers_data.erase(m_peers_data.begin() + i);
endRemoveRows();
}

if (m_peers_data.size() < new_peers_data.size()) {
// Some peers have been added to the end of the table.
beginInsertRows(QModelIndex(), m_peers_data.size(), new_peers_data.size() - 1);
m_peers_data.swap(new_peers_data);
endInsertRows();
} else {
m_peers_data.swap(new_peers_data);
}

Q_EMIT changed();
}
21 changes: 13 additions & 8 deletions src/qt/peertablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@
#include <net_processing.h> // For CNodeStateStats
#include <net.h>

#include <memory>

#include <QAbstractTableModel>
#include <QList>
#include <QModelIndex>
#include <QStringList>
#include <QVariant>

class PeerTablePriv;

Expand Down Expand Up @@ -61,18 +62,23 @@ class PeerTableModel : public QAbstractTableModel

/** @name Methods overridden from QAbstractTableModel
@{*/
int rowCount(const QModelIndex &parent) const override;
int columnCount(const QModelIndex &parent) const override;
QVariant data(const QModelIndex &index, int role) const override;
QVariant headerData(int section, Qt::Orientation orientation, int role) const override;
QModelIndex index(int row, int column, const QModelIndex &parent) const override;
int rowCount(const QModelIndex& parent = QModelIndex()) const override;
hebasto marked this conversation as resolved.
Show resolved Hide resolved
int columnCount(const QModelIndex& parent = QModelIndex()) const override;
QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
QModelIndex index(int row, int column, const QModelIndex& parent = QModelIndex()) const override;
Qt::ItemFlags flags(const QModelIndex &index) const override;
/*@}*/

public Q_SLOTS:
void refresh();

Q_SIGNALS:
void changed();

private:
//! Internal peer data structure.
QList<CNodeCombinedStats> m_peers_data{};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: {} seems redundant, it's not fundamental type like int, pointer or bool to need this kind of explicit default initialization, constructor will be invoked regardless.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is redundant. Leaving as is to be explicit.

interfaces::Node& m_node;
const QStringList columns{
/*: Title of Peers Table column which contains a
Expand All @@ -99,7 +105,6 @@ public Q_SLOTS:
/*: Title of Peers Table column which contains the peer's
User Agent string. */
tr("User Agent")};
std::unique_ptr<PeerTablePriv> priv;
QTimer *timer;
};

Expand Down
2 changes: 1 addition & 1 deletion src/qt/rpcconsole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_

// peer table signal handling - update peer details when selecting new node
connect(ui->peerWidget->selectionModel(), &QItemSelectionModel::selectionChanged, this, &RPCConsole::updateDetailWidget);
connect(model->getPeerTableModel(), &PeerTableModel::layoutChanged, this, &RPCConsole::updateDetailWidget);
connect(model->getPeerTableModel(), &PeerTableModel::changed, this, &RPCConsole::updateDetailWidget);

// set up ban table
ui->banlistWidget->setModel(model->getBanTableModel());
Expand Down