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: member

    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: member

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

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

  6. ryanofsky commented at 11:38 am on May 19, 2020: member

    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.

  7. jonasschnelli force-pushed on May 19, 2020
  8. jonasschnelli force-pushed on May 19, 2020
  9. jonasschnelli force-pushed on May 19, 2020
  10. 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.
  11. ryanofsky approved
  12. ryanofsky commented at 2:10 pm on May 19, 2020: member
    Code review ACK 4ef682a6601485a98ace43a9264e1e5bb03fca29. Works exactly as discussed, no issues at all that I could see
  13. jonasschnelli force-pushed on May 19, 2020
  14. 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").
  15. 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.
  16. ryanofsky commented at 6:23 pm on May 19, 2020: member
    Code review ACK ded3edd66c376eed6c9b6a703153d323e47aa4d7. Just apptests type registration added since last review
  17. DrahtBot added the label Needs rebase on May 21, 2020
  18. jonasschnelli force-pushed on May 22, 2020
  19. jonasschnelli commented at 7:58 am on May 22, 2020: contributor
    Rebased (due to #18740).
  20. DrahtBot removed the label Needs rebase on May 22, 2020
  21. 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 ;
  22. 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.
  23. promag commented at 9:41 am on May 22, 2020: member
    Concept ACK.
  24. ryanofsky approved
  25. ryanofsky commented at 5:44 pm on May 22, 2020: member
    Code review ACK bf788c761eca44084534da786e827c03b452a46f. No changes since last review other than rebase
  26. DrahtBot added the label Needs rebase on Jun 2, 2020
  27. promag commented at 10:53 am on June 2, 2020: member
    Rebased here aa70cc807aaf44987050da228c489ef00e4d5d5a, also dropped extra ;.
  28. jonasschnelli force-pushed on Jun 5, 2020
  29. jonasschnelli commented at 6:55 am on June 5, 2020: contributor
    Rebased and removed the extra ;
  30. DrahtBot removed the label Needs rebase on Jun 5, 2020
  31. DrahtBot added the label Needs rebase on Jul 3, 2020
  32. ryanofsky approved
  33. ryanofsky commented at 8:39 pm on July 8, 2020: member
    Code review ACK 43ef1f3b6f5349025794db0d263541b10f7b9223. No changes since last review other than rebase (qmetatype conflict) and removing double semicolon
  34. RPCConsole, take initial chaintip data as parameter 25e1d0bf41
  35. Add BlockAndHeaderTipInfo to the node interface/appInit b354a1480a
  36. Optionally populate BlockAndHeaderTipInfo during AppInitMain d42cb79068
  37. Reduce cs_main lock accumulation during GUI startup 386ec192a5
  38. jonasschnelli force-pushed on Aug 12, 2020
  39. jonasschnelli commented at 2:45 pm on August 12, 2020: contributor
    Rebased.
  40. DrahtBot removed the label Needs rebase on Aug 12, 2020
  41. ryanofsky approved
  42. ryanofsky commented at 10:50 pm on August 12, 2020: member
    Code review ACK 386ec192a57b76492125d691ceda1b4aa832312e. Just rebased since last review due to conflicts
  43. promag commented at 8:30 am on August 13, 2020: member
    Code review ACK 386ec192a57b76492125d691ceda1b4aa832312e.
  44. jonatack commented at 8:32 am on August 13, 2020: member
    Concept ACK
  45. jonasschnelli merged this on Aug 13, 2020
  46. jonasschnelli closed this on Aug 13, 2020
  47. Fabcien referenced this in commit ee8a058bda on Jul 6, 2021
  48. Fabcien referenced this in commit cc3538aeb2 on Jul 6, 2021
  49. Fabcien referenced this in commit faf4a486a6 on Jul 6, 2021
  50. DrahtBot locked this on Feb 15, 2022


jonasschnelli ryanofsky DrahtBot promag jonatack

Labels
GUI

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: 2024-12-18 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me