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.

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

[Qt] reduce cs_main locks during tip update, more fluently update UI #7112

Merged
merged 5 commits into from Nov 30, 2015

Conversation

Projects
None yet
4 participants
@jonasschnelli
Member

jonasschnelli commented Nov 27, 2015

At the moment, the UI gets often not updated during initial sync or a catch-up of serval blocks. The UI's try_lock often can't acquire the lock which result in a bad user experience ("Baby, it's not updating!").

This PR changes the blocktip-update signal parameter form a bare hash (uint256) to a CBlockIndex*, which allows to safely get it's height and timestamp without locking anything. In addition, the blocktip-update signal now gets fired also during initial sync, but with an extra boolean flag that indicates the initial sync state.
The PR temporarily has a benchmark for the tip update signal. On my standard system it uses 0.03ms for the signal (=reasonable). Keep in mind that this signal is synchronous, but it will emit another asynchronous QT signal to the UI thread that will care about the UI update!

The timer based update (every 250ms) is still active for bandwidth and mempool UI observation, although there no cs_main lock is required.

@jonasschnelli jonasschnelli added the GUI label Nov 27, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Nov 27, 2015

Status bar during IBD looks like: https://bitcoin.jonasschnelli.ch/qt/statusbar.mov
Console: https://bitcoin.jonasschnelli.ch/qt/console.mov
(sorry the german lang.)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 27, 2015

Member

Concept ACK, I've been wanting to do this for a long time

Member

laanwj commented Nov 27, 2015

Concept ACK, I've been wanting to do this for a long time

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 27, 2015

Member

Time-profiled this PR on a mac and it looks like that the performance is very similar to current master.
I try to now to implement a time delta throttling instead of emitting the signal if nHeight % 10 == 0 (only during initial sync).

Master:
bildschirmfoto 2015-11-27 um 11 06 55

PR:
bildschirmfoto 2015-11-27 um 11 05 40

Member

jonasschnelli commented Nov 27, 2015

Time-profiled this PR on a mac and it looks like that the performance is very similar to current master.
I try to now to implement a time delta throttling instead of emitting the signal if nHeight % 10 == 0 (only during initial sync).

Master:
bildschirmfoto 2015-11-27 um 11 06 55

PR:
bildschirmfoto 2015-11-27 um 11 05 40

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 27, 2015

Member

Updated. Much smaller diff/changeset now.
Changes to a min time delta update to not over-emit signals to the UI during IBD on a fast computer. Now the UI gets updated if the last update was more then 250ms ago.

Member

jonasschnelli commented Nov 27, 2015

Updated. Much smaller diff/changeset now.
Changes to a min time delta update to not over-emit signals to the UI during IBD on a fast computer. Now the UI gets updated if the last update was more then 250ms ago.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 27, 2015

Member

This fixes #5664 for me. Even during catching-up and heavy verification, it keeps updating the block number consistently.

Member

laanwj commented Nov 27, 2015

This fixes #5664 for me. Even during catching-up and heavy verification, it keeps updating the block number consistently.

}
// no locking required at this point
// the following calls will aquire the required lock
Q_EMIT mempoolSizeChanged(getMempoolSize(), getMempoolDynamicUsage());

This comment has been minimized.

@laanwj

laanwj Nov 27, 2015

Member

Hm, spoke too soon. It does hang on long contention of cs_main, I think here.
The TRY_LOCK should be re-added here.

@laanwj

laanwj Nov 27, 2015

Member

Hm, spoke too soon. It does hang on long contention of cs_main, I think here.
The TRY_LOCK should be re-added here.

This comment has been minimized.

@laanwj

laanwj Nov 27, 2015

Member

Hm no that can't be it, none of these require cs_main. Weird.

@laanwj

laanwj Nov 27, 2015

Member

Hm no that can't be it, none of these require cs_main. Weird.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 27, 2015

Member

Looks like it can still hang here:

#5  lock (this=<synthetic pointer>) at /store/orion/projects/bitcoin/experiment/boost/include/boost/thread/lock_types.hpp:346
#6  Enter (pszName=<optimized out>, pszFile=<optimized out>, nLine=<optimized out>, this=<synthetic pointer>) at ./sync.h:116
#7  CMutexLock (fTry=false, nLine=101, pszFile=0x5555559b7648 "qt/clientmodel.cpp", pszName=<optimized out>, mutexIn=..., this=<synthetic pointer>) at ./sync.h:137
#8  ClientModel::getVerificationProgress (this=<optimized out>) at qt/clientmodel.cpp:101
#9  0x00005555555db479 in BitcoinGUI::setNumBlocks (this=0x5555564ba3c0, count=375314, blockDate=...) at qt/bitcoingui.cpp:752
#10 0x000055555564960f in BitcoinGUI::qt_static_metacall (_o=0x5555564ba3c0, _c=<optimized out>, _id=<optimized out>, _a=<optimized out>) at qt/moc_bitcoingui.cpp:189

getVerificationProgress takes a lock on cs_main
I suppose because it uses chainActive.Tip()...
Maybe we can pass this progress in through a signal as well? After all it is only supposed to change when the block number does.

Member

laanwj commented Nov 27, 2015

Looks like it can still hang here:

#5  lock (this=<synthetic pointer>) at /store/orion/projects/bitcoin/experiment/boost/include/boost/thread/lock_types.hpp:346
#6  Enter (pszName=<optimized out>, pszFile=<optimized out>, nLine=<optimized out>, this=<synthetic pointer>) at ./sync.h:116
#7  CMutexLock (fTry=false, nLine=101, pszFile=0x5555559b7648 "qt/clientmodel.cpp", pszName=<optimized out>, mutexIn=..., this=<synthetic pointer>) at ./sync.h:137
#8  ClientModel::getVerificationProgress (this=<optimized out>) at qt/clientmodel.cpp:101
#9  0x00005555555db479 in BitcoinGUI::setNumBlocks (this=0x5555564ba3c0, count=375314, blockDate=...) at qt/bitcoingui.cpp:752
#10 0x000055555564960f in BitcoinGUI::qt_static_metacall (_o=0x5555564ba3c0, _c=<optimized out>, _id=<optimized out>, _a=<optimized out>) at qt/moc_bitcoingui.cpp:189

getVerificationProgress takes a lock on cs_main
I suppose because it uses chainActive.Tip()...
Maybe we can pass this progress in through a signal as well? After all it is only supposed to change when the block number does.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 27, 2015

Member

Concept ACK. f60a931d9137854f3bbcbc0589ab31563662c3c3 Looks a lot smoother on my system.

Member

MarcoFalke commented Nov 27, 2015

Concept ACK. f60a931d9137854f3bbcbc0589ab31563662c3c3 Looks a lot smoother on my system.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 27, 2015

Member

Added another commit that solves the getVerificationProgress() locking issue.
The only reason why getVerificationProgress() needs a cs_main lock is because it accesses chainActive.Tip().
The last commit will pass the signals CBlockIndex* to the GuessVerificationProgress() which make the whole UI update cs_main lock free.

Member

jonasschnelli commented Nov 27, 2015

Added another commit that solves the getVerificationProgress() locking issue.
The only reason why getVerificationProgress() needs a cs_main lock is because it accesses chainActive.Tip().
The last commit will pass the signals CBlockIndex* to the GuessVerificationProgress() which make the whole UI update cs_main lock free.

@jonasschnelli jonasschnelli added this to the 0.12.0 milestone Nov 27, 2015

@laanwj

View changes

Show outdated Hide outdated src/qt/bitcoingui.cpp
@@ -672,7 +672,7 @@ void BitcoinGUI::setNumConnections(int count)
labelConnectionsIcon->setToolTip(tr("%n active connection(s) to Bitcoin network", "", count));
}
void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate)
void BitcoinGUI::setNumBlocks(int count, const QDateTime& blockDate, const CBlockIndex *tip)

This comment has been minimized.

@laanwj

laanwj Nov 27, 2015

Member

I don't like passing the tip through the view code. Let's just pass the verification progress directly
(could even remove ClientModel::getVerificationProgress() after that)

@laanwj

laanwj Nov 27, 2015

Member

I don't like passing the tip through the view code. Let's just pass the verification progress directly
(could even remove ClientModel::getVerificationProgress() after that)

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 27, 2015

Member

Hmm... but i don't want to execute getVerificationProgress() during the synchronous core signal. I guess it's better to call this function over the UI thread.
But let me have a closer look...

@jonasschnelli

jonasschnelli Nov 27, 2015

Member

Hmm... but i don't want to execute getVerificationProgress() during the synchronous core signal. I guess it's better to call this function over the UI thread.
But let me have a closer look...

This comment has been minimized.

@laanwj

laanwj Nov 27, 2015

Member

Hmm okay you have a point.
But ideally these kind of things happen in the model (ClientModel) in this case, not in the view class. But that'd require 'intercepting' the signal there to add this information.
Don't like how core's handles leak all over the place now.
But yes maybe better to live with this for now...

@laanwj

laanwj Nov 27, 2015

Member

Hmm okay you have a point.
But ideally these kind of things happen in the model (ClientModel) in this case, not in the view class. But that'd require 'intercepting' the signal there to add this information.
Don't like how core's handles leak all over the place now.
But yes maybe better to live with this for now...

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 27, 2015

Member

@jonasschnelli @laanwj I benchmark GuessVerificationProgress (+random fetch from chainActive) here at around 80ns. I think it's perfectly acceptable to compute it before passing to the GUI.

Member

sipa commented Nov 27, 2015

@jonasschnelli @laanwj I benchmark GuessVerificationProgress (+random fetch from chainActive) here at around 80ns. I think it's perfectly acceptable to compute it before passing to the GUI.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 27, 2015

Member

@sipa thanks; absolutely, in that case we should call it in the signal handler directly before sendingto the GUI. I expect GetTimeMillis() takes longer than that.

Member

laanwj commented Nov 27, 2015

@sipa thanks; absolutely, in that case we should call it in the signal handler directly before sendingto the GUI. I expect GetTimeMillis() takes longer than that.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 27, 2015

Member

@sipa: Thanks for the info.

Just added another commit that calculate the verification progress during the synchronous core signal, and passes a double through the UI signal.
Feels very fast now (testes IBD, short catch-up 10 blocks).

Member

jonasschnelli commented Nov 27, 2015

@sipa: Thanks for the info.

Just added another commit that calculate the verification progress during the synchronous core signal, and passes a double through the UI signal.
Feels very fast now (testes IBD, short catch-up 10 blocks).

@jonasschnelli

View changes

Show outdated Hide outdated src/qt/clientmodel.cpp
CBlockIndex *tip = (CBlockIndex *)tipIn;
if (!tip)
{
LOCK(cs_main);

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 27, 2015

Member

This lock is only required when initially update the UI (first time, outside of the core signal scope).

@jonasschnelli

jonasschnelli Nov 27, 2015

Member

This lock is only required when initially update the UI (first time, outside of the core signal scope).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 27, 2015

Member

Binares are built if someone likes to test this: https://bitcoin.jonasschnelli.ch/pulls/7112/

Member

jonasschnelli commented Nov 27, 2015

Binares are built if someone likes to test this: https://bitcoin.jonasschnelli.ch/pulls/7112/

@sipa

View changes

Show outdated Hide outdated src/qt/clientmodel.cpp
{
LOCK(cs_main);
return Checkpoints::GuessVerificationProgress(Params().Checkpoints(), chainActive.Tip());
CBlockIndex *tip = (CBlockIndex *)tipIn;

This comment has been minimized.

@sipa

sipa Nov 28, 2015

Member

Don't recast to non-const, and certainly not with a C-style cast.

@sipa

sipa Nov 28, 2015

Member

Don't recast to non-const, and certainly not with a C-style cast.

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 28, 2015

Member

Better like this (it's now const_cast<CBlockIndex *>(tipIn))?
Or would it make more sense to pass a non-const through the core signal?

@jonasschnelli

jonasschnelli Nov 28, 2015

Member

Better like this (it's now const_cast<CBlockIndex *>(tipIn))?
Or would it make more sense to pass a non-const through the core signal?

This comment has been minimized.

@sipa

sipa Nov 28, 2015

Member

No, I think GuessVerificationProgress should take a const CBlockIndex* :)

@sipa

sipa Nov 28, 2015

Member

No, I think GuessVerificationProgress should take a const CBlockIndex* :)

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 28, 2015

Member

Yeah. Would be nice.. but i guess this would be a too-broad change for this PR scope.

@jonasschnelli

jonasschnelli Nov 28, 2015

Member

Yeah. Would be nice.. but i guess this would be a too-broad change for this PR scope.

This comment has been minimized.

@sipa

sipa Nov 28, 2015

Member

Just change the argument to const there. No other changes needed (I tried) :)

@sipa

sipa Nov 28, 2015

Member

Just change the argument to const there. No other changes needed (I tried) :)

@sipa

View changes

Show outdated Hide outdated src/main.cpp
}
//TODO: remove tip notification benchmark

This comment has been minimized.

@sipa

sipa Nov 28, 2015

Member

Maybe remove this again :)

@sipa

sipa Nov 28, 2015

Member

Maybe remove this again :)

jonasschnelli added some commits Nov 26, 2015

NotifyBlockTip signal: switch from hash (uint256) to CBlockIndex*
- also adds a boolean for indication if the tip update was happening during initial sync
- emit notification also during initial sync
@jonasschnelli

View changes

Show outdated Hide outdated src/main.cpp
}
}
if (!vHashes.empty()) {
uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);
}

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 30, 2015

Member

@sdaftuar: I'm not familiar with the block announcements with headers, could you check if this rebase makes sense?

@jonasschnelli

jonasschnelli Nov 30, 2015

Member

@sdaftuar: I'm not familiar with the block announcements with headers, could you check if this rebase makes sense?

Show outdated Hide outdated src/main.cpp
}
}
// Always notify the UI if a new block tip was connected
uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 30, 2015

Member

@sdaftuar: I'm not familiar with the block announcements with headers, could you check if this rebase makes sense?
I'm firing off this event even when vHashes is empty now.

@jonasschnelli

jonasschnelli Nov 30, 2015

Member

@sdaftuar: I'm not familiar with the block announcements with headers, could you check if this rebase makes sense?
I'm firing off this event even when vHashes is empty now.

This comment has been minimized.

@sipa

sipa Nov 30, 2015

Member
@sipa

sipa via email Nov 30, 2015

Member

This comment has been minimized.

@sipa

sipa Nov 30, 2015

Member
@sipa

sipa via email Nov 30, 2015

Member
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 30, 2015

Member

Rebased. Removed notification benchmark.

Member

jonasschnelli commented Nov 30, 2015

Rebased. Removed notification benchmark.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 30, 2015

Member

ACK. Works great now. The spinner is actually spinning again while syncing instead of blundering and glitching along :)

Member

laanwj commented Nov 30, 2015

ACK. Works great now. The spinner is actually spinning again while syncing instead of blundering and glitching along :)

Move uiInterface.NotifyBlockTip signal above the core/wallet signal
- This will keep getbestblockhash more in sync with blocknotify callbacks
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 30, 2015

Member

Added a commit that resolves conflicts with block header announcements (#7129) and now also includes PR #7037 (because it conflicts with it).

Member

jonasschnelli commented Nov 30, 2015

Added a commit that resolves conflicts with block header announcements (#7129) and now also includes PR #7037 (because it conflicts with it).

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 30, 2015

Member

utACK non-GUI code.

Member

sipa commented Nov 30, 2015

utACK non-GUI code.

@laanwj laanwj merged commit 9af5f9c into bitcoin:master Nov 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 30, 2015

Merge pull request #7112
9af5f9c Move uiInterface.NotifyBlockTip signal above the core/wallet signal - This will keep getbestblockhash more in sync with blocknotify callbacks (Jonas Schnelli)
4082e46 [Qt] call GuessVerificationProgress synchronous during core signal, pass double over UI signal (Jonas Schnelli)
947d20b [Qt] reduce cs_main in getVerificationProgress() (Jonas Schnelli)
e6d50fc [Qt] update block tip (height and date) without locking cs_main, update always (each block) (Jonas Schnelli)
012fc91 NotifyBlockTip signal: switch from hash (uint256) to CBlockIndex* - also adds a boolean for indication if the tip update was happening during initial sync - emit notification also during initial sync (Jonas Schnelli)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment