Reduce cs_main lock accumulation during GUI startup #19011

jonasschnelli · 2020-05-18T20:14:18Z

During the GUI startup, there is currently an accumulation of cs_main locks due to setting initial chain state values at multiple locations (in the GUI main thread).

This PR tries to cache the initial chain state (tip height, tip time, best header, etc.) short after loading the blockindex.

The cached values are then used instead of fetching them again (and thus locking cs_main) during setting the client model.

This should fix the initial GUI blocking often experienced during or short after the splashscreen.
On mac, best tested together with #19007.

ryanofsky · 2020-05-18T22:15:16Z

Concept ACK and approach half-ACK. This seems like a good change to make with the caveat that it shouldn't be necessary to add new global NodeContext variables and add new node initialization and caching code to fix a GUI problem.

Instead of having AppInitMain set global variables in NodeContext that get returned by a new Node::getInitialChainState method, and having the node cache information about itself on behalf of the GUI, I think the GUI should be able to query the node at the right time and pass the information where it needs to go during its initialization.

This would mean instead of having a new Node::getInitialChainState method returning cached information, there would be a new Node::getBlockTip method [1] returning live information. The GUI could call this right after it calls Node::appInitMain, and pass the returned values along to setClientModel. It might also be good to define a BlockTip struct to avoid repeating the same function arguments multiple places.

[1] Suggesting getBlockTip as a method name to be consistent with other existing method names: getHeaderTip, handleNotifyBlockTip, handleNotifyHeaderTip as well as the new struct BlockTip from #17993

DrahtBot · 2020-05-18T23:55:26Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Remove confusing BlockIndex global #19413 (refactor: Remove confusing BlockIndex global by MarcoFalke)
refactor: remove ::vpwallets and related global variables #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
refactor: Move wallet methods out of chain.h and node.h #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
test: Remove duplicate NodeContext hacks #19098 (test: Remove duplicate NodeContext hacks by ryanofsky)
gui: Improve thread naming #18790 (gui: Improve thread naming by hebasto)
qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
qt: Do not block GUI thread in RPCConsole #17659 (qt: Do not block GUI thread in RPCConsole by hebasto)
Add new bitcoin_rw.conf file that is used for settings modified by this software itself #11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jonasschnelli · 2020-05-19T07:23:09Z

This would mean instead of having a new Node::getInitialChainState method returning cached information, there would be a new Node::getBlockTip method [1] returning live information. The GUI could call this right after it calls Node::appInitMain, and pass the returned values along to setClientModel. It might also be good to define a BlockTip struct to avoid repeating the same function arguments multiple places.

I thought about this and agree that this PRs solution might not be very elegant.

The approach you described should work,.. the main difference is that the networking is running then already which might lead to additional cs_main locking (within this PR, we fetch the information before we start the networking). Your approach would also require at least another locking of cs_main (right?).

What I was looking for was using an existing cs_main lock to gather the information required for the GUI (avoid collecting the same information into concurrency-safe primitives over and over).

ryanofsky · 2020-05-19T11:38:05Z

The approach you described should work,.. the main difference is that the networking is running then already which might lead to additional cs_main locking (within this PR, we fetch the information before we start the networking). Your approach would also require at least another locking of cs_main (right?).

Oh, I thought I might have been missing something. Now I understand why this is saving tip information inside AppInitMain. I think you're right that there is a possibility of cs_main blocking if information is retrieved after the AppInitMain call instead of during it (though in this case it still wouldn't be happening on the qt event loop thread, so might not be an actual problem).

If this is a problem, it could be solved most directly by having AppInitMain just return tip information, instead of stashing it into a global struct to be retrieved later. Using something like struct BlockTip from #17993 and having AppInitMain take an optional BlockTip* output argument would be a pretty clean way for it to return information.

jonasschnelli · 2020-05-19T13:21:37Z

Thanks for the feedback @ryanofsky.
I tried to implement your approach without the global struct.

ryanofsky

Code review ACK 4ef682a. Works exactly as discussed, no issues at all that I could see

jonasschnelli · 2020-05-19T17:57:07Z

Force pushed a fix for the qt test apptests (missed the new qRegisterMetaType<interfaces::BlockAndHeaderTipInfo>("interfaces::BlockAndHeaderTipInfo").

ryanofsky

Code review ACK ded3edd. Just apptests type registration added since last review

ryanofsky · 2020-05-19T18:22:48Z

src/qt/test/apptests.cpp

@@ -66,6 +66,7 @@ void AppTests::appTests()
    ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
    LogInstance().DisconnectTestLogger();

+    qRegisterMetaType<interfaces::BlockAndHeaderTipInfo>("interfaces::BlockAndHeaderTipInfo");


In commit "Reduce cs_main lock accumulation during GUI startup" (ded3edd)

I guess it'd be nice to register types in a common location in the future to avoid the need to do this twice

Agree, suggestion GUIUtils::registerMetaTypes or something like that.

Yes. Lets do this later when more registration are required/appear.

jonasschnelli · 2020-05-22T07:58:16Z

Rebased (due to #18740).

promag

Concept ACK.

promag · 2020-05-22T09:31:51Z

src/init.cpp

+            tip_info->verification_progress = GuessVerificationProgress(Params().TxData(), ::ChainActive().Tip());
+        }
+        if (tip_info && ::pindexBestHeader) {
+            tip_info->header_height = ::pindexBestHeader->nHeight;;


nit double ;

promag · 2020-05-22T09:34:49Z

src/qt/test/apptests.cpp

@@ -66,6 +66,7 @@ void AppTests::appTests()
    ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
    LogInstance().DisconnectTestLogger();

+    qRegisterMetaType<interfaces::BlockAndHeaderTipInfo>("interfaces::BlockAndHeaderTipInfo");


Agree, suggestion GUIUtils::registerMetaTypes or something like that.

src/qt/bitcoingui.cpp

ryanofsky

Code review ACK bf788c7. No changes since last review other than rebase

promag

Rebased here aa70cc8, also dropped extra ;.

jonasschnelli · 2020-06-05T06:55:10Z

Rebased and removed the extra ;

ryanofsky

Code review ACK 43ef1f3. No changes since last review other than rebase (qmetatype conflict) and removing double semicolon

jonasschnelli · 2020-08-12T14:45:23Z

Rebased.

ryanofsky

Code review ACK 386ec19. Just rebased since last review due to conflicts

promag · 2020-08-13T08:30:31Z

Code review ACK 386ec19.

jonatack · 2020-08-13T08:32:56Z

Concept ACK

Summary: PR description: Reduce cs_main lock accumulation during GUI startup > During the GUI startup, there is currently an accumulation of cs_main locks due to setting initial chain state values at multiple locations (in the GUI main thread). > > This PR tries to cache the initial chain state (tip height, tip time, best header, etc.) short after loading the blockindex. > > The cached values are then used instead of fetching them again (and thus locking cs_main) during setting the client model. > > This should fix the initial GUI blocking often experienced during or short after the splashscreen. This is a backport of [[bitcoin/bitcoin#19011 | core#19011]] [1/4] bitcoin/bitcoin@25e1d0b Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9734

Summary: PR description: > During the GUI startup, there is currently an accumulation of cs_main locks due to setting initial chain state values at multiple locations (in the GUI main thread). > > This PR tries to cache the initial chain state (tip height, tip time, best header, etc.) short after loading the blockindex. > > The cached values are then used instead of fetching them again (and thus locking cs_main) during setting the client model. > > This should fix the initial GUI blocking often experienced during or short after the splashscreen. bitcoin/bitcoin@b354a14 > Add BlockAndHeaderTipInfo to the node interface/appInit bitcoin/bitcoin@d42cb79 > Optionally populate BlockAndHeaderTipInfo during AppInitMain This is a backport of [[bitcoin/bitcoin#19011 | core#19011]] [2&3/4] The commits needed squashing because the first one alone breaks the build. Depends on D9734 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9735

Summary: PR description: > During the GUI startup, there is currently an accumulation of cs_main locks due to setting initial chain state values at multiple locations (in the GUI main thread). > > This PR tries to cache the initial chain state (tip height, tip time, best header, etc.) short after loading the blockindex. > > The cached values are then used instead of fetching them again (and thus locking cs_main) during setting the client model. > > This should fix the initial GUI blocking often experienced during or short after the splashscreen. This concludes backport of [[bitcoin/bitcoin#19011 | core#19011]] [4/4] bitcoin/bitcoin@386ec19 Depends on D9735 Depends on D9737 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D9736

jonasschnelli added the GUI label May 18, 2020

DrahtBot mentioned this pull request May 19, 2020

relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload #19007

Closed

jonasschnelli force-pushed the 2020/05/guilocks branch 3 times, most recently from 2cf87fe to 4ef682a Compare May 19, 2020 13:20

DrahtBot mentioned this pull request May 19, 2020

qt: Use SynchronizationState enum for signals to GUI #18152

Merged

ryanofsky approved these changes May 19, 2020

View reviewed changes

jonasschnelli force-pushed the 2020/05/guilocks branch from 4ef682a to ded3edd Compare May 19, 2020 17:55

ryanofsky reviewed May 19, 2020

View reviewed changes

This was referenced May 19, 2020

gui: Improve thread naming #18790

Merged

Remove g_rpc_node global #18740

Merged

qt: Do not block GUI thread in RPCConsole #17659

Closed

DrahtBot added the Needs rebase label May 21, 2020

jonasschnelli force-pushed the 2020/05/guilocks branch from ded3edd to bf788c7 Compare May 22, 2020 07:57

DrahtBot removed the Needs rebase label May 22, 2020

promag reviewed May 22, 2020

View reviewed changes

ryanofsky approved these changes May 22, 2020

View reviewed changes

maflcko mentioned this pull request May 27, 2020

doc: Separate repository for the gui #19071

Merged

DrahtBot added the Needs rebase label Jun 2, 2020

promag reviewed Jun 2, 2020

View reviewed changes

DrahtBot mentioned this pull request Jun 2, 2020

Multiprocess bitcoin #10102

Draft

jonasschnelli force-pushed the 2020/05/guilocks branch from bf788c7 to 43ef1f3 Compare June 5, 2020 06:50

DrahtBot removed the Needs rebase label Jun 5, 2020

This was referenced Jun 5, 2020

multiprocess: Add basic spawn and IPC support #19160

Merged

qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877

Closed

DrahtBot mentioned this pull request Jun 29, 2020

refactor: Remove confusing BlockIndex global #19413

Merged

DrahtBot added the Needs rebase label Jul 3, 2020

ryanofsky approved these changes Jul 8, 2020

View reviewed changes

jonasschnelli added 4 commits August 12, 2020 16:31

RPCConsole, take initial chaintip data as parameter

25e1d0b

Add BlockAndHeaderTipInfo to the node interface/appInit

b354a14

Optionally populate BlockAndHeaderTipInfo during AppInitMain

d42cb79

Reduce cs_main lock accumulation during GUI startup

386ec19

jonasschnelli force-pushed the 2020/05/guilocks branch from 43ef1f3 to 386ec19 Compare August 12, 2020 14:45

DrahtBot removed the Needs rebase label Aug 12, 2020

ryanofsky approved these changes Aug 12, 2020

View reviewed changes

jonasschnelli merged commit 1052b09 into bitcoin:master Aug 13, 2020

hebasto mentioned this pull request Aug 13, 2020

util: Make default arg values more specific #19471

Closed

ryanofsky mentioned this pull request Aug 17, 2020

Parse params directly instead of through node (partial revert #10244) bitcoin-core/gui#35

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Reduce cs_main lock accumulation during GUI startup #19011

Reduce cs_main lock accumulation during GUI startup #19011

Uh oh!

Conversation

jonasschnelli commented May 18, 2020

Uh oh!

ryanofsky commented May 18, 2020

Uh oh!

DrahtBot commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonasschnelli commented May 19, 2020

Uh oh!

ryanofsky commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented May 19, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 19, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 19, 2020

Choose a reason for hiding this comment

Uh oh!

promag May 22, 2020

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Aug 13, 2020

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 22, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag May 22, 2020

Choose a reason for hiding this comment

Uh oh!

promag May 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jun 5, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 12, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 13, 2020

Uh oh!

jonatack commented Aug 13, 2020

Uh oh!

Uh oh!

DrahtBot commented May 18, 2020 •

edited

Loading

ryanofsky commented May 19, 2020 •

edited

Loading