New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: make `gettxoutsettinfo` run lock-free #6290

Merged
merged 1 commit into from Jun 19, 2015

Conversation

Projects
None yet
5 participants
@laanwj
Member

laanwj commented Jun 16, 2015

For leveldb "An iterator operates on a snapshot of the database taken when the iterator is created". This means that it is unnecessary to lock out other threads while computing statistics, and neither to hold cs_main for the whole time. Let the thread run free.

I've been using this patch for about 8 months without issues, including on a node that automatically dumps the information for every N blocks.

rpc: make `gettxoutsettinfo` run lock-free
For leveldb "An iterator operates on a snapshot of the database taken
when the iterator is created". This means that it is unnecessary to
lock out other threads while computing statistics, and neither to hold
cs_main for the whole time. Let the thread run free.

@laanwj laanwj added the RPC/REST/ZMQ label Jun 16, 2015

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 16, 2015

Member

Untested ACK

Member

sipa commented Jun 16, 2015

Untested ACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 16, 2015

Contributor

ut ACK

Contributor

jgarzik commented Jun 16, 2015

ut ACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 17, 2015

Member

This would mean that CoinsViewCache::GetBestBlock() (which accesses mutable uint256 hashBlock in "self" space) get called unlocked (https://github.com/bitcoin/bitcoin/pull/6290/files#diff-81e4f16a1b5d5b7ca25351a63d07cb80R111). But i'm not sure it this is a problem.

But i agree that we should do more of these LOCK space reducing.

Member

jonasschnelli commented Jun 17, 2015

This would mean that CoinsViewCache::GetBestBlock() (which accesses mutable uint256 hashBlock in "self" space) get called unlocked (https://github.com/bitcoin/bitcoin/pull/6290/files#diff-81e4f16a1b5d5b7ca25351a63d07cb80R111). But i'm not sure it this is a problem.

But i agree that we should do more of these LOCK space reducing.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 17, 2015

Member

This would mean that CoinsViewCache::GetBestBlock() (which accesses mutable uint256 hashBlock in "self" space) get called unlocked (https://github.com/bitcoin/bitcoin/pull/6290/files#diff-81e4f16a1b5d5b7ca25351a63d07cb80R111). But i'm not sure it this is a problem.

Whoa. Good catch. I expected that this would invoke CCoinsViewDB::GetBestBlock() which accesses just the db and not any internal fields or global state. But it's a virtual. It shouldn't actually be using the cached value at all but query the db.

Member

laanwj commented Jun 17, 2015

This would mean that CoinsViewCache::GetBestBlock() (which accesses mutable uint256 hashBlock in "self" space) get called unlocked (https://github.com/bitcoin/bitcoin/pull/6290/files#diff-81e4f16a1b5d5b7ca25351a63d07cb80R111). But i'm not sure it this is a problem.

Whoa. Good catch. I expected that this would invoke CCoinsViewDB::GetBestBlock() which accesses just the db and not any internal fields or global state. But it's a virtual. It shouldn't actually be using the cached value at all but query the db.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 17, 2015

Member

Correction: CCoinsViewCache does not derive from CCoinsViewDB (or the other way around), it just wraps one. So there is no problem AFAIK.

Member

laanwj commented Jun 17, 2015

Correction: CCoinsViewCache does not derive from CCoinsViewDB (or the other way around), it just wraps one. So there is no problem AFAIK.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 17, 2015

Member

Right. Was following the wrong path... it calls uint256 CCoinsViewDB::GetBestBlock(). Sorry for the noise.

Member

jonasschnelli commented Jun 17, 2015

Right. Was following the wrong path... it calls uint256 CCoinsViewDB::GetBestBlock(). Sorry for the noise.

@laanwj laanwj merged commit 57092ed into bitcoin:master Jun 19, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 19, 2015

Merge pull request #6290
57092ed rpc: make `gettxoutsettinfo` run lock-free (Wladimir J. van der Laan)
{
LOCK(cs_main);
stats.nHeight = mapBlockIndex.find(stats.hashBlock)->second->nHeight;
}

This comment has been minimized.

@luke-jr

luke-jr Jun 23, 2015

Member

Couldn't nHeight change before we get to this block, after we start getting the other stats?

@luke-jr

luke-jr Jun 23, 2015

Member

Couldn't nHeight change before we get to this block, after we start getting the other stats?

This comment has been minimized.

@sipa

sipa Jun 23, 2015

Member
@sipa

sipa via email Jun 23, 2015

Member

This comment has been minimized.

@luke-jr

luke-jr Jun 23, 2015

Member

Ah, right.

@luke-jr

luke-jr Jun 23, 2015

Member

Ah, right.

@str4d str4d referenced this pull request Feb 14, 2017

Merged

Bitcoin 0.12 RPC PRs 1 #2100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment