Reduce cs_main lock accumulation during GUI startup #19011

pull jonasschnelli wants to merge 4 commits into bitcoin:master from jonasschnelli:2020/05/guilocks changing 11 files +49 −24
  1. jonasschnelli commented at 8:14 PM on May 18, 2020: contributor

    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.

  2. jonasschnelli added the label GUI on May 18, 2020
  3. ryanofsky commented at 10:15 PM on May 18, 2020: contributor

    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

  4. DrahtBot commented at 11:55 PM on May 18, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19413 (refactor: Remove confusing BlockIndex global by MarcoFalke)
    • #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
    • #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
    • #19098 (test: Remove duplicate NodeContext hacks by ryanofsky)
    • #18790 (gui: Improve thread naming by hebasto)
    • #17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
    • #17659 (qt: Do not block GUI thread in RPCConsole by hebasto)
    • #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.

  5. DrahtBot cross-referenced this on May 19, 2020 from issue relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload by jonasschnelli
  6. jonasschnelli commented at 7:23 AM on May 19, 2020: contributor

    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).

  7. ryanofsky commented at 11:38 AM on May 19, 2020: contributor

    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.

  8. jonasschnelli force-pushed on May 19, 2020
  9. jonasschnelli force-pushed on May 19, 2020
  10. jonasschnelli force-pushed on May 19, 2020
  11. jonasschnelli commented at 1:21 PM on May 19, 2020: contributor

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

  12. DrahtBot cross-referenced this on May 19, 2020 from issue qt: Use SynchronizationState enum for signals to GUI by hebasto
  13. ryanofsky approved
  14. ryanofsky commented at 2:10 PM on May 19, 2020: contributor

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

  15. jonasschnelli force-pushed on May 19, 2020
  16. jonasschnelli commented at 5:57 PM on May 19, 2020: contributor

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

  17. in src/qt/test/apptests.cpp:70 in ded3edd66c outdated 
      65 | @@ -66,6 +66,7 @@ void AppTests::appTests()
  66 |      ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
  67 |      LogInstance().DisconnectTestLogger();
  68 |  
  69 | +    qRegisterMetaType<interfaces::BlockAndHeaderTipInfo>("interfaces::BlockAndHeaderTipInfo");
    ryanofsky commented at 6:22 PM on May 19, 2020:

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

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

    promag commented at 9:34 AM on May 22, 2020:

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

    jonasschnelli commented at 8:29 AM on August 13, 2020:

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

  18. ryanofsky commented at 6:23 PM on May 19, 2020: contributor

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

  19. DrahtBot cross-referenced this on May 19, 2020 from issue gui: Improve thread naming by hebasto
  20. DrahtBot cross-referenced this on May 19, 2020 from issue Remove g_rpc_node global by ryanofsky
  21. DrahtBot cross-referenced this on May 20, 2020 from issue qt: Do not block GUI thread in RPCConsole by hebasto
  22. DrahtBot added the label Needs rebase on May 21, 2020
  23. jonasschnelli force-pushed on May 22, 2020
  24. jonasschnelli commented at 7:58 AM on May 22, 2020: contributor

    Rebased (due to #18740).

  25. DrahtBot removed the label Needs rebase on May 22, 2020
  26. in src/init.cpp:1878 in bf788c761e outdated 
    1873 | +            tip_info->block_height = chain_active_height;
1874 | +            tip_info->block_time = ::ChainActive().Tip() ? ::ChainActive().Tip()->GetBlockTime() : Params().GenesisBlock().GetBlockTime();
1875 | +            tip_info->verification_progress = GuessVerificationProgress(Params().TxData(), ::ChainActive().Tip());
1876 | +        }
1877 | +        if (tip_info && ::pindexBestHeader) {
1878 | +            tip_info->header_height = ::pindexBestHeader->nHeight;;
    promag commented at 9:31 AM on May 22, 2020:

    nit double ;

  27. in src/qt/bitcoingui.cpp:603 in bf788c761e outdated 
     578 | @@ -579,7 +579,7 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel)
 579 |          // Show progress dialog
 580 |          connect(_clientModel, &ClientModel::showProgress, this, &BitcoinGUI::showProgress);
 581 |  
 582 | -        rpcConsole->setClientModel(_clientModel);
 583 | +        rpcConsole->setClientModel(_clientModel, tip_info->block_height, tip_info->block_time, tip_info->verification_progress);
    promag commented at 9:40 AM on May 22, 2020:

    Why not pass tip_info too?

    jonasschnelli commented at 8:28 AM on August 13, 2020:

    I think it's fine to pass primitives at this level to be more flexible with structure changes.

  28. promag commented at 9:41 AM on May 22, 2020: member

    Concept ACK.

  29. ryanofsky approved
  30. ryanofsky commented at 5:44 PM on May 22, 2020: contributor

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

  31. MarcoFalke cross-referenced this on May 27, 2020 from issue doc: Separate repository for the gui by MarcoFalke
  32. DrahtBot cross-referenced this on May 29, 2020 from issue refactor: remove ::vpwallets and related global variables by ryanofsky
  33. DrahtBot cross-referenced this on May 29, 2020 from issue refactor: Move wallet methods out of chain.h and node.h by ryanofsky
  34. DrahtBot cross-referenced this on May 29, 2020 from issue test: Remove duplicate NodeContext hacks by ryanofsky
  35. DrahtBot cross-referenced this on May 29, 2020 from issue gui, refactor: Register Qt meta types in application constructor by promag
  36. DrahtBot cross-referenced this on May 30, 2020 from issue Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr
  37. DrahtBot added the label Needs rebase on Jun 2, 2020
  38. promag commented at 10:53 AM on June 2, 2020: member

    Rebased here aa70cc807aaf44987050da228c489ef00e4d5d5a, also dropped extra ;.

  39. DrahtBot cross-referenced this on Jun 2, 2020 from issue Multiprocess bitcoin by ryanofsky
  40. jonasschnelli force-pushed on Jun 5, 2020
  41. jonasschnelli commented at 6:55 AM on June 5, 2020: contributor

    Rebased and removed the extra ;

  42. DrahtBot removed the label Needs rebase on Jun 5, 2020
  43. DrahtBot cross-referenced this on Jun 5, 2020 from issue multiprocess: Add basic spawn and IPC support by ryanofsky
  44. DrahtBot cross-referenced this on Jun 5, 2020 from issue qt, refactor: Make BitcoinUnits::Unit a scoped enum by hebasto
  45. DrahtBot cross-referenced this on Jun 29, 2020 from issue refactor: Remove confusing BlockIndex global by MarcoFalke
  46. DrahtBot added the label Needs rebase on Jul 3, 2020
  47. ryanofsky approved
  48. ryanofsky commented at 8:39 PM on July 8, 2020: contributor

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

  49. RPCConsole, take initial chaintip data as parameter 25e1d0bf41
  50. Add BlockAndHeaderTipInfo to the node interface/appInit b354a1480a
  51. Optionally populate BlockAndHeaderTipInfo during AppInitMain d42cb79068
  52. Reduce cs_main lock accumulation during GUI startup 386ec192a5
  53. jonasschnelli force-pushed on Aug 12, 2020
  54. jonasschnelli commented at 2:45 PM on August 12, 2020: contributor

    Rebased.

  55. DrahtBot removed the label Needs rebase on Aug 12, 2020
  56. ryanofsky approved
  57. ryanofsky commented at 10:50 PM on August 12, 2020: contributor

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

  58. promag commented at 8:30 AM on August 13, 2020: member

    Code review ACK 386ec192a57b76492125d691ceda1b4aa832312e.

  59. jonatack commented at 8:32 AM on August 13, 2020: contributor

    Concept ACK

  60. jonasschnelli merged this on Aug 13, 2020
  61. jonasschnelli closed this on Aug 13, 2020
  62. hebasto cross-referenced this on Aug 13, 2020 from issue util: Make default arg values more specific by hebasto
  63. ryanofsky cross-referenced this on Aug 17, 2020 from issue Parse params directly instead of through node (partial revert #10244) by ryanofsky
  64. Fabcien referenced this in commit ee8a058bda on Jul 6, 2021
  65. Fabcien referenced this in commit cc3538aeb2 on Jul 6, 2021
  66. Fabcien referenced this in commit faf4a486a6 on Jul 6, 2021
  67. bitcoin locked this on Feb 15, 2022
Contributors
jonasschnelliryanofskyDrahtBotpromagjonatack
Labels
GUI
Linked (view graph)
#17993 gui: Balance/TxStatus polling update based on last block hash.#19007 relax GUI locks: avoid unnecesarry calls to ::ChainstateActive().IsInitialBlockDownload#19071 doc: Separate repository for the gui#19471 util: Make default arg values more specific
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 11:54 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me