[Qt] Reduce a significant cs_main lock freeze #10231

1. 
   jonasschnelli commented at 4:05 PM on April 19, 2017: contributor

   I can't remember why we added this in the first place: but in current master, we request a cs_main lock (not a try lock) every block tip update in order to get the best headers height / time. This results in significant freezes in the GUI during IBD/catch-up.

   This PR adds two atomic caches for the best headers height and time.

   If we do a 0.14.1rc3, we should consider adding this.

2. jonasschnelli added the label GUI on Apr 19, 2017
3. jonasschnelli added the label Needs backport on Apr 19, 2017
4. 
   TheBlueMatt commented at 4:34 PM on April 19, 2017: member

   Can the GUI instead cache the current best tip? This may help with @ryanofsky's process split as well.

5. 
   jonasschnelli commented at 4:34 PM on April 19, 2017: contributor

   @TheBlueMatt: Yes. @theuni recommended this on IRC and I'm currently changing it.

6. jonasschnelli force-pushed on Apr 19, 2017
7. 
   jonasschnelli commented at 4:52 PM on April 19, 2017: contributor

   Moved the cache from the core validation logic to the GUI.

8. in src/qt/clientmodel.cpp:77 in 745a27fa53 outdated

     77 |  {
     78 | -    LOCK(cs_main);
     79 | -    if (!pindexBestHeader)
     80 | -        return 0;
     81 | -    return pindexBestHeader->nHeight;
     82 | +    if (cachedBestHeaderHeight == 0) {
   

   ---

   ---

   

   gmaxwell commented at 5:28 PM on April 19, 2017:

   Where is this initialized? -- these don't have static duration AFAICT.

   Also is zero really the best value to use? -- it's in range, so it'll keep relocking again while there is no data there.

9. 
   gmaxwell commented at 5:29 PM on April 19, 2017: contributor

   What does the GUI use the header time/height for as opposed to the tip?

10. jonasschnelli force-pushed on Apr 19, 2017
11. Reduce cs_main locks during modal overlay by adding an atomic cache 7148f5e7d7
12. jonasschnelli force-pushed on Apr 19, 2017
13. Update the remaining blocks left in modaloverlay at init. cf92bce526
14. 
    jonasschnelli commented at 6:55 PM on April 19, 2017: contributor

    Thanks @gmaxwell. Added the missing initialisation and switched to -1 as indicator wether the cache is set or not.

    What does the GUI use the header time/height for as opposed to the tip?

    The only use case is to calculate the remaining blocks to download/validate (modal sync progress overlay).

15. in src/qt/clientmodel.cpp:77 in cf92bce526 outdated

      73 | @@ -72,20 +74,28 @@ int ClientModel::getNumBlocks() const
      74 |      return chainActive.Height();
      75 |  }
      76 |  
      77 | -int ClientModel::getHeaderTipHeight() const
      78 | +int ClientModel::getHeaderTipHeight()
    

    ---

    ---

    

    luke-jr commented at 7:07 PM on April 19, 2017:

    Maybe this should remain const, and declare the caches mutable?

16. in src/qt/clientmodel.cpp:84 in cf92bce526 outdated

      84 | +    if (cachedBestHeaderHeight == -1) {
      85 | +        // make sure we initially populate the cache via a cs_main lock
      86 | +        // otherwise we need to wait for a tip update
      87 | +        LOCK(cs_main);
      88 | +        if (pindexBestHeader) {
      89 | +            cachedBestHeaderHeight = pindexBestHeader->nHeight;
    

    ---

    ---

    

    luke-jr commented at 7:09 PM on April 19, 2017:

    Would it make sense to populate both caches at once perhaps?

    ---

    

    jonasschnelli commented at 7:11 PM on April 19, 2017:

    Good idea.

17. luke-jr approved
18. 
    luke-jr commented at 7:09 PM on April 19, 2017: member

    utACK

19. 
    TheBlueMatt commented at 8:20 PM on April 19, 2017: member

    utACK

20. gmaxwell approved
21. 
    gmaxwell commented at 4:40 AM on April 20, 2017: contributor

    utACK

22. Declare headers height/time cache mutable, re-set the methods const 610a91719c
23. Set both time/height header caches at the same time 928d4a9ac5
24. 
    jonasschnelli commented at 7:55 AM on April 20, 2017: contributor

    Followed @luke-jr's advice and re-set the method to const and declared the cache atomic's mutable. Also, both caches are now getting set regardless of the called method. Yes, you could argue to have an explicit function to set the cache to avoid code duplication. But for two lines it's not worth in my opinion. @gmaxwell, @TheBlueMatt: thanks for a re-ack. :)

25. Add missing <atomic> header in clientmodel.h 4082fb0003
26. 
    laanwj commented at 11:41 AM on April 20, 2017: member

    Less blocking of the gui on cs_main is always a good thing, thanks! utACK 4082fb0

27. laanwj merged this on Apr 20, 2017
28. laanwj closed this on Apr 20, 2017

    ---

29. laanwj referenced this in commit 987a6c0956 on Apr 20, 2017
30. luke-jr referenced this in commit 213ee4fd77 on Apr 21, 2017
31. luke-jr referenced this in commit 1e435ed4f1 on Apr 21, 2017
32. luke-jr referenced this in commit b19ecc40e6 on Apr 21, 2017
33. luke-jr referenced this in commit 4c9ec9758d on Apr 21, 2017
34. luke-jr referenced this in commit e8350f9f6b on Apr 21, 2017
35. MarcoFalke added this to the milestone 0.14.2 on Apr 23, 2017
36. laanwj removed the label Needs backport on May 31, 2017
37. codablock referenced this in commit 8b3c39fb6b on Oct 31, 2017
38. codablock referenced this in commit 746d487ddb on Oct 31, 2017
39. UdjinM6 referenced this in commit d23adcc0f1 on Oct 31, 2017
40. MarcoFalke locked this on Sep 8, 2021

Contributors

jonasschnelliTheBlueMattgmaxwellluke-jrlaanwj

Milestone
0.14.2