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

Jump to bottom

Ditch wallet model juggling #417

Closed
wants to merge 2 commits into from
Closed

Ditch wallet model juggling #417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3e7047
qt: Ditch wallet model juggling
promag Sep 2, 2021
e2ffc98
qt: Move wallet preloading out of model constructors
promag Sep 3, 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
17 changes: 11 additions & 6 deletions src/qt/addresstablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,13 @@ class AddressTablePriv
{
public:
QList<AddressTableEntry> cachedAddressTable;
AddressTableModel *parent;
AddressTableModel* const parent;
const bool pk_hash_only;

explicit AddressTablePriv(AddressTableModel *_parent):
parent(_parent) {}
explicit AddressTablePriv(AddressTableModel *_parent, bool pk_hash_only):
parent(_parent), pk_hash_only(pk_hash_only) {}

void refreshAddressTable(interfaces::Wallet& wallet, bool pk_hash_only = false)
void refreshAddressTable(interfaces::Wallet& wallet)
{
cachedAddressTable.clear();
{
Expand Down Expand Up @@ -166,15 +167,19 @@ AddressTableModel::AddressTableModel(WalletModel *parent, bool pk_hash_only) :
QAbstractTableModel(parent), walletModel(parent)
{
columns << tr("Label") << tr("Address");
priv = new AddressTablePriv(this);
priv->refreshAddressTable(parent->wallet(), pk_hash_only);
priv = new AddressTablePriv(this, pk_hash_only);
}

AddressTableModel::~AddressTableModel()
{
delete priv;
}

void AddressTableModel::preload()
{
priv->refreshAddressTable(walletModel->wallet());
}

int AddressTableModel::rowCount(const QModelIndex &parent) const
{
if (parent.isValid()) {
Expand Down
2 changes: 2 additions & 0 deletions src/qt/addresstablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class AddressTableModel : public QAbstractTableModel
static const QString Send; /**< Specifies send address */
static const QString Receive; /**< Specifies receive address */

void preload();

/** @name Methods overridden from QAbstractTableModel
@{*/
int rowCount(const QModelIndex &parent) const override;
Expand Down
13 changes: 8 additions & 5 deletions src/qt/recentrequeststablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@
RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
QAbstractTableModel(parent), walletModel(parent)
{
// Load entries from wallet
for (const std::string& request : parent->wallet().getAddressReceiveRequests()) {
addNewRequest(request);
}

/* These columns must match the indices in the ColumnIndex enumeration */
columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();

Expand All @@ -36,6 +31,14 @@ RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :

RecentRequestsTableModel::~RecentRequestsTableModel() = default;

void RecentRequestsTableModel::preload()
{
// Load entries from wallet
for (const std::string& request : walletModel->wallet().getAddressReceiveRequests()) {
addNewRequest(request);
}
}

int RecentRequestsTableModel::rowCount(const QModelIndex &parent) const
{
if (parent.isValid()) {
Expand Down
2 changes: 2 additions & 0 deletions src/qt/recentrequeststablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ class RecentRequestsTableModel: public QAbstractTableModel
NUMBER_OF_COLUMNS
};

void preload();

/** @name Methods overridden from QAbstractTableModel
@{*/
int rowCount(const QModelIndex &parent) const override;
Expand Down
1 change: 1 addition & 0 deletions src/qt/test/addressbooktests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node)
WalletContext& context = *node.walletLoader().context();
AddWallet(context, wallet);
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
walletModel.preload();
RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt);
EditAddressDialog editAddressDialog(EditAddressDialog::NewSendingAddress);
editAddressDialog.setModel(walletModel.getAddressTableModel());
Expand Down
1 change: 1 addition & 0 deletions src/qt/test/wallettests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ void TestGUI(interfaces::Node& node)
WalletContext& context = *node.walletLoader().context();
AddWallet(context, wallet);
WalletModel walletModel(interfaces::MakeWallet(context, wallet), clientModel, platformStyle.get());
walletModel.preload();
RemoveWallet(context, wallet, /* load_on_start= */ std::nullopt);
sendCoinsDialog.setModel(&walletModel);
transactionView.setModel(&walletModel);
Expand Down
10 changes: 6 additions & 4 deletions src/qt/transactiontablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,7 @@ TransactionTableModel::TransactionTableModel(const PlatformStyle *_platformStyle
fProcessingQueuedTransactions(false),
platformStyle(_platformStyle)
{
subscribeToCoreSignals();

columns << QString() << QString() << tr("Date") << tr("Type") << tr("Label") << BitcoinUnits::getAmountColumnTitle(walletModel->getOptionsModel()->getDisplayUnit());
priv->refreshWallet(walletModel->wallet());

connect(walletModel->getOptionsModel(), &OptionsModel::displayUnitChanged, this, &TransactionTableModel::updateDisplayUnit);
}

Expand All @@ -270,6 +266,12 @@ TransactionTableModel::~TransactionTableModel()
delete priv;
}

void TransactionTableModel::preload()
{
subscribeToCoreSignals();
priv->refreshWallet(walletModel->wallet());
}

/** Updates the column title to "Amount (DisplayUnit)" and emits headerDataChanged() signal for table headers to react. */
void TransactionTableModel::updateAmountColumnTitle()
{
Expand Down
2 changes: 2 additions & 0 deletions src/qt/transactiontablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ class TransactionTableModel : public QAbstractTableModel
RawDecorationRole,
};

void preload();

int rowCount(const QModelIndex &parent) const override;
int columnCount(const QModelIndex &parent) const override;
QVariant data(const QModelIndex &index, int role) const override;
Expand Down
19 changes: 6 additions & 13 deletions src/qt/walletcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,21 +125,14 @@ WalletModel* WalletController::getOrCreateWallet(std::unique_ptr<interfaces::Wal
}
}

// Instantiate model and register it.
WalletModel* wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style,
nullptr /* required for the following moveToThread() call */);

// Move WalletModel object to the thread that created the WalletController
// object (GUI main thread), instead of the current thread, which could be
// an outside wallet thread or RPC thread sending a LoadWallet notification.
// This ensures queued signals sent to the WalletModel object will be
// handled on the GUI event loop.
wallet_model->moveToThread(thread());
// setParent(parent) must be called in the thread which created the parent object. More details in #18948.
QMetaObject::invokeMethod(this, [wallet_model, this] {
wallet_model->setParent(this);
WalletModel* wallet_model;
// Instantiate model in GUI main thread.
Copy link
Contributor

Choose a reason for hiding this comment

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

In commit "qt: Ditch wallet model juggling" (f3e7047)

It seems unfortunate that this commit is deleting previous comment which explained what this code was trying to do, replacing it with a much shorter comment that doesn't provide much explanation.

It would be good to combine two comments with something like:

// Instantiate WalletModel in the thread that created the WalletControllet object (GUI main thread), instead of the current thread, which could be an outside wallet thread or RPC thread sending a LoadWallet notification. This ensures queued signals sent to the WalletModel object will be handled on the GUI event loop.

I also think commit message is unclear about what behavior is changing. Initially I didn't realize that constructing the WalletModel was so expensive and actually loaded transactions, so this at first appeared to be a no-op change that was replacing setParent and moveToThread calls with a constructor call. Would suggest adding an explanation like the following to the commit message:

qt: Ditch wallet model juggling

Construct WalletModel on the GUI thread, not on the outside thread which loaded the wallet. This is a simplification because it avoids the need to make setParent and moveToThread calls after the wallet is constructed. This commit however can cause GUI freezes while loading the wallet because the WalletModel constructor can take a long time to run if the wallet contains a lot of transactions. The GUI freeze issue is addressed by making WalletModel initialization more asynchronous in the next commit."

Or, as an alternative, if you switch the order of the two commits, you could drop "This commit however can cause GUI freezes..." part and everything after.

QMetaObject::invokeMethod(this, [&] {
wallet_model = new WalletModel(std::move(wallet), m_client_model, m_platform_style, this);
}, GUIUtil::blockingGUIThreadConnection());

wallet_model->preload();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a follow-up, some preload will be replaced by lazy loading.

Copy link
Contributor

Choose a reason for hiding this comment

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

In commit "qt: Move wallet preloading out of model constructors" (e2ffc98)

I think it would be good to switch the order of the two commits so the other commit doesn't cause a performance regression, and so wallet preloading doesn't move from the outside thread to the GUI thread than back to the outside thread again within the PR.

Additionally might suggest renaming preload to loadData to give a clearer idea what this method is actually doing and avoid the "pre" prefix, which I'm not sure actually means anything, since it runs separately after the constructor so might more logically be called "postload".

Also would add a rationale to the commit description like:

The allows WalletModel constructor to be called on the GUI thread (so queued signals sent to the wallet model object will execute on the GUI thread), while allowing wallet data to be loaded off the GUI thread, so the GUI will not freeze while the data is loading.


m_wallets.push_back(wallet_model);

// WalletModel::startPollBalance needs to be called in a thread managed by
Expand Down
10 changes: 8 additions & 2 deletions src/qt/walletmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,21 @@ WalletModel::WalletModel(std::unique_ptr<interfaces::Wallet> wallet, ClientModel
addressTableModel = new AddressTableModel(this);
transactionTableModel = new TransactionTableModel(platformStyle, this);
recentRequestsTableModel = new RecentRequestsTableModel(this);

subscribeToCoreSignals();
}

WalletModel::~WalletModel()
{
unsubscribeFromCoreSignals();
}

void WalletModel::preload()
{
addressTableModel->preload();
transactionTableModel->preload();
recentRequestsTableModel->preload();
subscribeToCoreSignals();
}

void WalletModel::startPollBalance()
{
// This timer will be fired repeatedly to update the balance
Expand Down
2 changes: 2 additions & 0 deletions src/qt/walletmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ class WalletModel : public QObject
Unlocked // wallet->IsCrypted() && !wallet->IsLocked()
};

void preload();

OptionsModel* getOptionsModel() const;
AddressTableModel* getAddressTableModel() const;
TransactionTableModel* getTransactionTableModel() const;
Expand Down