Reduce cs_main locks during ConnectTip/SyncWithWallets by jonasschnelli · Pull Request #7946 · bitcoin/bitcoin

jonasschnelli · 2016-04-26T12:03:07Z

DisconnectTip and AcceptToMempool are still holding cs_main (not intended to improve this in this PR)

sipa · 2016-04-26T12:05:52Z

src/main.cpp

This is not correct, it needs to iterate through all the blocks that got connected, not just the new tip.

jonasschnelli · 2016-04-26T13:42:48Z

As always when you want to improve the locking a little bit, the change gets quickly more invasive.

Update serval things:

Fixed @sipa point (iterate though all connected blocks).
Removed the CBlock * parameter from the SyncTransaction signal and added CBlockIndex* together with int posInBlock.
Updated the wallet logic that it can handle the slightly different signal.

kazcw · 2016-04-27T14:17:16Z

ActivateBestChainStep reuses a vector for ConnectTip's txConflicted to combine results from multiple blocks. ConnectTip passes txConflicted to mempoool.removeForBlock every time.

kazcw · 2016-04-27T14:19:52Z

src/main.cpp

Could put the new loops below this comment with the other non-cs_main notifications

jonasschnelli · 2016-05-06T09:48:09Z

@kazcw Thanks for the detailed code review!

Added a squashme commit with fixed for issues found by @kazcw.

kazcw · 2016-05-06T13:57:38Z

You know what, I misread. txConflicted is an out-only parameter all the way down the call stack, so it was fine without the txConflictedCurrent. Sorry about that.

sipa · 2016-05-25T14:18:50Z

Needs rebase.

sipa · 2016-05-25T14:19:48Z

src/main.cpp

Use std::tuple instead?

jonasschnelli · 2016-05-25T14:34:57Z

Rebased, fixed sipas found issue and switched to std::tuple

sipa · 2016-05-25T14:36:24Z

src/main.cpp

Why a temporary variable, and not just pass vchConflicted to mempool.removeForBlock()? txConflictedCurrent is not used elsewhere.

Without a temporary variable, mempool.removeForBlock would always work though the combined list txConflicted that was initialized here.

Although I'm not sure if this would be a problem, but at least it would slightly change current masters behavior.

"Work through"? All that function does is append deleted CTransactions to it.

Yes. Your right. Removed the temp. var.

sipa · 2016-05-25T15:44:53Z

src/main.cpp

Sorry for changing my mind, but can you define a struct for this instead? Larger tuples don't really have good readability.

IMO tuple is ideal for a such use case. It's only a temporary variable and its scope is very small. IMO an custom struct seems to be over complex.

Fair enough.

c++ needs collections.namedtuple :)

http://www.boost.org/doc/libs/1_58_0/libs/fusion/doc/html/fusion/container/map.html

given the repeated use, maybe just typedef the std::tuple? tuple has some nice conveniences over a struct alternative.

sipa · 2016-05-25T17:21:28Z

src/wallet/wallet.cpp

This cs_main is unfortunate, but I guess we can get rid of it in a follow-up.

sipa · 2016-05-25T17:22:04Z

utACK fdf8a5efe03004f4a581ba41b87737a4e1f1ff44

dcousens · 2016-06-03T00:32:29Z

utACK fdf8a5e

laanwj · 2016-06-10T13:32:46Z

utACK fdf8a5e, but the squashme commits need squashing

jonasschnelli · 2016-06-10T15:09:57Z

Squashed.

This runs BlockConnectedDisconnected, TransactionAddedToMempool, SetBestChain, and Inventory on the background scheduler thread. This partially reverts bitcoin#9583, re-enabling some of the gains from bitcoin#7946. This does not, however, re-enable the gains achieved by repeatedly releasing cs_main between each transaction processed.

jonasschnelli added the Refactoring label Apr 26, 2016

sipa reviewed Apr 26, 2016
View reviewed changes

src/main.cpp Outdated

Copy link

Member

sipa Apr 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct, it needs to iterate through all the blocks that got connected, not just the new tip.

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch 2 times, most recently from fdbee7d to 22d9380 Compare April 26, 2016 13:31

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch 2 times, most recently from 6416d91 to 086654e Compare April 26, 2016 20:25

kazcw reviewed Apr 27, 2016
View reviewed changes

src/main.cpp Outdated

Copy link

Contributor

kazcw Apr 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could put the new loops below this comment with the other non-cs_main notifications

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from 6fc0be6 to e8ea3dd Compare May 6, 2016 09:47

sipa reviewed May 25, 2016
View reviewed changes

src/main.cpp Outdated

Copy link

Member

sipa May 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use std::tuple instead?

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from e8ea3dd to 9471f32 Compare May 25, 2016 14:33

sipa reviewed May 25, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from 9471f32 to 580a665 Compare May 25, 2016 14:47

sipa reviewed May 25, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from 580a665 to fdf8a5e Compare May 25, 2016 15:58

sipa reviewed May 25, 2016
View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Member

sipa May 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cs_main is unfortunate, but I guess we can get rid of it in a follow-up.

jonasschnelli force-pushed the 2016/04/cs_main_wallet branch from fdf8a5e to d31dd19 Compare June 10, 2016 15:09

TheBlueMatt mentioned this pull request Apr 27, 2017

Call wallet notify callbacks in scheduler thread (without cs_main) #10286

Merged

furszy mentioned this pull request Jul 1, 2020

[Validation] Reduce cs_main locks during ConnectTip/SyncWithWallets PIVX-Project/PIVX#1721

Merged

Conversation

jonasschnelli commented Apr 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 26, 2016

Uh oh!

kazcw commented Apr 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 6, 2016

Uh oh!

kazcw commented May 6, 2016

Uh oh!

sipa commented May 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli May 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 25, 2016

Uh oh!

dcousens commented Jun 3, 2016

Uh oh!

laanwj commented Jun 10, 2016

Uh oh!

jonasschnelli commented Jun 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jonasschnelli commented May 25, 2016 •

edited

Loading

sipa May 25, 2016 •

edited

Loading

jonasschnelli May 25, 2016 •

edited

Loading