/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Qt] Reduce a significant cs_main lock freeze #10231

Merged
merged 5 commits into from Apr 20, 2017
+32 −10

Conversation

Reviewers

@gmaxwell gmaxwell

@luke-jr luke-jr

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
  0.14.2
6 participants
@jonasschnelli @TheBlueMatt @gmaxwell @laanwj @luke-jr @MarcoFalke
@jonasschnelli
Member

jonasschnelli commented Apr 19, 2017 •
Edited 1 time

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.

@jonasschnelli jonasschnelli added GUI Needs backport labels Apr 19, 2017

@TheBlueMatt
Contributor

TheBlueMatt commented Apr 19, 2017

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

jonasschnelli commented Apr 19, 2017

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

jonasschnelli commented Apr 19, 2017

Moved the cache from the core validation logic to the GUI.
src/qt/clientmodel.cpp
- if (!pindexBestHeader)
- return 0;
- return pindexBestHeader->nHeight;
+ if (cachedBestHeaderHeight == 0) {
@gmaxwell

gmaxwell Apr 19, 2017

Member

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.

@gmaxwell
Member

gmaxwell commented Apr 19, 2017

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

jonasschnelli added some commits Apr 19, 2017

@jonasschnelli
Reduce cs_main locks during modal overlay by adding an atomic cache
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
7148f5e
@jonasschnelli
Update the remaining blocks left in modaloverlay at init.
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
cf92bce
@jonasschnelli
Member

jonasschnelli commented Apr 19, 2017

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).
@luke-jr

luke-jr approved these changes Apr 19, 2017
View changes

utACK

src/qt/clientmodel.cpp
@@ -72,20 +74,28 @@ int ClientModel::getNumBlocks() const
return chainActive.Height();
}
-int ClientModel::getHeaderTipHeight() const
+int ClientModel::getHeaderTipHeight()
@luke-jr

luke-jr Apr 19, 2017

Member

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

src/qt/clientmodel.cpp
+ // otherwise we need to wait for a tip update
+ LOCK(cs_main);
+ if (pindexBestHeader) {
+ cachedBestHeaderHeight = pindexBestHeader->nHeight;
@luke-jr

luke-jr Apr 19, 2017

Member

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

@jonasschnelli

jonasschnelli Apr 19, 2017

Member

Good idea.

@TheBlueMatt
Contributor

TheBlueMatt commented Apr 19, 2017

utACK
@gmaxwell

gmaxwell approved these changes Apr 20, 2017
View changes

utACK

jonasschnelli added some commits Apr 20, 2017

@jonasschnelli
Declare headers height/time cache mutable, re-set the methods const
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
610a917
@jonasschnelli
Set both time/height header caches at the same time
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
928d4a9
@jonasschnelli
Member

jonasschnelli commented Apr 20, 2017

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. :)
@jonasschnelli
Add missing <atomic> header in clientmodel.h
This commit was signed with a verified signature.
@jonasschnelli jonasschnelli Jonas Schnelli
GPG key ID: 1EB776BB03C7922D Learn about signing commits
4082fb0
@laanwj
Owner

laanwj commented Apr 20, 2017

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

@laanwj laanwj merged commit 4082fb0 into bitcoin:master Apr 20, 2017
1 check was pending

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Apr 20, 2017

@laanwj
Merge #10231: [Qt] Reduce a significant cs_main lock freeze 
4082fb0 Add missing <atomic> header in clientmodel.h (Jonas Schnelli)
928d4a9 Set both time/height header caches at the same time (Jonas Schnelli)
610a917 Declare headers height/time cache mutable, re-set the methods const (Jonas Schnelli)
cf92bce Update the remaining blocks left in modaloverlay at init. (Jonas Schnelli)
7148f5e Reduce cs_main locks during modal overlay by adding an atomic cache (Jonas Schnelli)

Tree-SHA512: a92ca22f90b8b2a5e8eb94fdce531ef44542e21a8dbbb0693f7723d7018592cb68de687a2a0aac91d31cbf019793f8e922550656d2b130ed3d854d60630341db
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 74810B012346C9A6 Learn about signing commits
987a6c0

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

@jonasschnelli @luke-jr
Reduce cs_main locks during modal overlay by adding an atomic cache 
Github-Pull: #10231
Rebased-From: 7148f5e
213ee4f

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

@jonasschnelli @luke-jr
Update the remaining blocks left in modaloverlay at init. 
Github-Pull: #10231
Rebased-From: cf92bce
1e435ed

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

@jonasschnelli @luke-jr
Declare headers height/time cache mutable, re-set the methods const 
Github-Pull: #10231
Rebased-From: 610a917
b19ecc4

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

@jonasschnelli @luke-jr
Set both time/height header caches at the same time 
Github-Pull: #10231
Rebased-From: 928d4a9
4c9ec97

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Apr 21, 2017

@jonasschnelli @luke-jr
Add missing <atomic> header in clientmodel.h 
Github-Pull: #10231
Rebased-From: 4082fb0
e8350f9

@MarcoFalke MarcoFalke added this to the 0.14.2 milestone Apr 23, 2017

This was referenced May 31, 2017

Open
Merged

@laanwj laanwj removed the Needs backport label May 31, 2017

@laanwj laanwj referenced this pull request Jun 27, 2017

Open

Windows Gui becomes non responsive after "Done loading" #10646

codablock added a commit to codablock/dash that referenced this pull request Oct 31, 2017

@laanwj @codablock
Merge #10231: [Qt] Reduce a significant cs_main lock freeze 
4082fb0 Add missing <atomic> header in clientmodel.h (Jonas Schnelli)
928d4a9 Set both time/height header caches at the same time (Jonas Schnelli)
610a917 Declare headers height/time cache mutable, re-set the methods const (Jonas Schnelli)
cf92bce Update the remaining blocks left in modaloverlay at init. (Jonas Schnelli)
7148f5e Reduce cs_main locks during modal overlay by adding an atomic cache (Jonas Schnelli)

Tree-SHA512: a92ca22f90b8b2a5e8eb94fdce531ef44542e21a8dbbb0693f7723d7018592cb68de687a2a0aac91d31cbf019793f8e922550656d2b130ed3d854d60630341db
8b3c39f

@codablock codablock referenced this pull request in dashpay/dash Oct 31, 2017

Merged

Backport #10231: [Qt] Reduce a significant cs_main lock freeze #1704

codablock added a commit to codablock/dash that referenced this pull request Oct 31, 2017

@laanwj @codablock
Merge #10231: [Qt] Reduce a significant cs_main lock freeze 
4082fb0 Add missing <atomic> header in clientmodel.h (Jonas Schnelli)
928d4a9 Set both time/height header caches at the same time (Jonas Schnelli)
610a917 Declare headers height/time cache mutable, re-set the methods const (Jonas Schnelli)
cf92bce Update the remaining blocks left in modaloverlay at init. (Jonas Schnelli)
7148f5e Reduce cs_main locks during modal overlay by adding an atomic cache (Jonas Schnelli)

Tree-SHA512: a92ca22f90b8b2a5e8eb94fdce531ef44542e21a8dbbb0693f7723d7018592cb68de687a2a0aac91d31cbf019793f8e922550656d2b130ed3d854d60630341db
746d487

UdjinM6 added a commit to dashpay/dash that referenced this pull request Oct 31, 2017

@codablock @UdjinM6
Merge #10231: [Qt] Reduce a significant cs_main lock freeze (#1704) 
4082fb0 Add missing <atomic> header in clientmodel.h (Jonas Schnelli)
928d4a9 Set both time/height header caches at the same time (Jonas Schnelli)
610a917 Declare headers height/time cache mutable, re-set the methods const (Jonas Schnelli)
cf92bce Update the remaining blocks left in modaloverlay at init. (Jonas Schnelli)
7148f5e Reduce cs_main locks during modal overlay by adding an atomic cache (Jonas Schnelli)

Tree-SHA512: a92ca22f90b8b2a5e8eb94fdce531ef44542e21a8dbbb0693f7723d7018592cb68de687a2a0aac91d31cbf019793f8e922550656d2b130ed3d854d60630341db
d23adcc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment