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

Make sigcache faster, more efficient, larger #6918

Merged
merged 3 commits into from Nov 12, 2015

Conversation

Projects
None yet
9 participants
@sipa
Member

sipa commented Oct 30, 2015

  • Changes the -maxsigcachesize argument from entries to megabytes
  • Change the default to 40 MiB (as opposed to ~10 MiB before), corresponding to over 500000 entries on 64-bit systems.
  • Store salted SHA256 hashes in the cache, rather than full entries.
  • Switch implementation from std::set to boost::unordered_set (smaller and faster)
  • Remove cache entries after having been validated inside a block, to keep the cache fresh.
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Oct 30, 2015

Member

Concept ACK

Sent from my iPhone

On Oct 30, 2015, at 6:20 PM, Pieter Wuille notifications@github.com wrote:

Changes the -maxsigcachesize argument from entries to megabytes
Change the default to 40 MiB (as opposed to ~10 MiB before), corresponding to over 500000 entries on 64-bit systems.
Store salted SHA256 hashes in the cache, rather than full entries.
Switch implementation from std::set to boost::unordered_set (smaller and faster)
You can view, comment on, or merge this pull request online at:

#6918

Commit Summary

Make sigcache faster and more efficient
File Changes

M src/init.cpp (3)
M src/script/sigcache.cpp (80)
M src/script/sigcache.h (4)
Patch Links:

https://github.com/bitcoin/bitcoin/pull/6918.patch
https://github.com/bitcoin/bitcoin/pull/6918.diff

Reply to this email directly or view it on GitHub.

Member

morcos commented Oct 30, 2015

Concept ACK

Sent from my iPhone

On Oct 30, 2015, at 6:20 PM, Pieter Wuille notifications@github.com wrote:

Changes the -maxsigcachesize argument from entries to megabytes
Change the default to 40 MiB (as opposed to ~10 MiB before), corresponding to over 500000 entries on 64-bit systems.
Store salted SHA256 hashes in the cache, rather than full entries.
Switch implementation from std::set to boost::unordered_set (smaller and faster)
You can view, comment on, or merge this pull request online at:

#6918

Commit Summary

Make sigcache faster and more efficient
File Changes

M src/init.cpp (3)
M src/script/sigcache.cpp (80)
M src/script/sigcache.h (4)
Patch Links:

https://github.com/bitcoin/bitcoin/pull/6918.patch
https://github.com/bitcoin/bitcoin/pull/6918.diff

Reply to this email directly or view it on GitHub.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Oct 30, 2015

Contributor

concept ACK

Contributor

pstratem commented Oct 30, 2015

concept ACK

@laanwj laanwj added the Validation label Oct 30, 2015

@dcousens

View changes

Show outdated Hide outdated src/script/sigcache.cpp
boost::unique_lock<boost::shared_mutex> lock(cs_sigcache);
while (static_cast<int64_t>(setValid.size()) > nMaxCacheSize)
while (memusage::DynamicUsage(setValid) > nMaxSize)

This comment has been minimized.

@dcousens

dcousens Oct 30, 2015

Contributor

OOI, what is the time complexity of memusage::DynamicUsage?

@dcousens

dcousens Oct 30, 2015

Contributor

OOI, what is the time complexity of memusage::DynamicUsage?

This comment has been minimized.

@sipa

sipa Oct 30, 2015

Member

O(1).

@sipa

sipa Oct 30, 2015

Member

O(1).

}
sigdata_type k(hash, vchSig, pubKey);
setValid.insert(k);
setValid.insert(entry);

This comment has been minimized.

@dcousens

dcousens Oct 30, 2015

Contributor

nit: technically, this would mean it could briefly be greater than the allowed memory limits.

@dcousens

dcousens Oct 30, 2015

Contributor

nit: technically, this would mean it could briefly be greater than the allowed memory limits.

This comment has been minimized.

@sipa

sipa Oct 30, 2015

Member

correct, but we're talking about ~80 bytes...

@sipa

sipa Oct 30, 2015

Member

correct, but we're talking about ~80 bytes...

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 30, 2015

Contributor

concept ACK, utACK 👍

Contributor

dcousens commented Oct 30, 2015

concept ACK, utACK 👍

bool
Get(const uint256 &hash, const std::vector<unsigned char>& vchSig, const CPubKey& pubKey)
Get(const uint256& entry)

This comment has been minimized.

@dcousens

dcousens Oct 30, 2015

Contributor

Get is such a weird name, its really more like Contains?

@dcousens

dcousens Oct 30, 2015

Contributor

Get is such a weird name, its really more like Contains?

This comment has been minimized.

@instagibbs

instagibbs Nov 2, 2015

Member

I think it's going for a getter/setter paradigm(see: Set function below), but I think the names could indeed be improved. It's confusing to have "Set" be both the type of data structure and the primary way of updating it, imo.

@instagibbs

instagibbs Nov 2, 2015

Member

I think it's going for a getter/setter paradigm(see: Set function below), but I think the names could indeed be improved. It's confusing to have "Set" be both the type of data structure and the primary way of updating it, imo.

{
boost::shared_lock<boost::shared_mutex> lock(cs_sigcache);
return setValid.count(entry);

This comment has been minimized.

@dcousens

dcousens Oct 30, 2015

Contributor

std::static_cast<bool>(count) or count > 0?

@dcousens

dcousens Oct 30, 2015

Contributor

std::static_cast<bool>(count) or count > 0?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 31, 2015

Member

Added a commit that removes cache entries after they've been seen in a block, so the cache remains fresh.

Member

sipa commented Oct 31, 2015

Added a commit that removes cache entries after they've been seen in a block, so the cache remains fresh.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 31, 2015

Contributor

re-ACK

Contributor

dcousens commented Oct 31, 2015

re-ACK

class CSignatureCacheHasher
{
public:
size_t operator()(const uint256& key) const {

This comment has been minimized.

@Diapolo

Diapolo Oct 31, 2015

Just a qustion to understand the motivation, why is here the opening bracket in the same line and below (line 48) it's on a new line?

@Diapolo

Diapolo Oct 31, 2015

Just a qustion to understand the motivation, why is here the opening bracket in the same line and below (line 48) it's on a new line?

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 1, 2015

"A bucket is a slot in the container's internal hash table to which elements are assigned based on their hash value.", I suspect this will not give you a uniform member of the set. Edit: Hm. no, you're using the bucket iterator below (not helpfully named.)

"A bucket is a slot in the container's internal hash table to which elements are assigned based on their hash value.", I suspect this will not give you a uniform member of the set. Edit: Hm. no, you're using the bucket iterator below (not helpfully named.)

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 1, 2015

Owner

It certainly is not uniform given a known hash function. The effective hash function itself is (presumably) indistinguishable from random due to the 256-bit per-cache salt.

Owner

sipa replied Nov 1, 2015

It certainly is not uniform given a known hash function. The effective hash function itself is (presumably) indistinguishable from random due to the 256-bit per-cache salt.

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 1, 2015

Owner

It is, however, slightly biased towards removing more recent values, as those are placed (I expect) at the beginning of the linked lists within the bucket. I can iterate to remove the last element in the bucket instead, biasing it in favor of removing older entries.

Owner

sipa replied Nov 1, 2015

It is, however, slightly biased towards removing more recent values, as those are placed (I expect) at the beginning of the linked lists within the bucket. I can iterate to remove the last element in the bucket instead, biasing it in favor of removing older entries.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 1, 2015

I don't see how this avoids the trial validation in getblocktemplate from wiping out the cache.

gmaxwell commented on 0b9e9dc Nov 1, 2015

I don't see how this avoids the trial validation in getblocktemplate from wiping out the cache.

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 2, 2015

Owner

It doesn't.

Owner

sipa replied Nov 2, 2015

It doesn't.

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 2, 2015

It needs to, otherwise this change will trash validation performance for miners.

gmaxwell replied Nov 2, 2015

It needs to, otherwise this change will trash validation performance for miners.

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 2, 2015

Owner

Fixed.

Owner

sipa replied Nov 2, 2015

Fixed.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 2, 2015

Member

Added a commit to not wipe the cache from TBV.

Member

sipa commented Nov 2, 2015

Added a commit to not wipe the cache from TBV.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 2, 2015

Member

Currently gathering statistics about the cache size - hitrate relation.

Member

sipa commented Nov 2, 2015

Currently gathering statistics about the cache size - hitrate relation.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Nov 2, 2015

Member

utACK

Member

instagibbs commented Nov 2, 2015

utACK

@gmaxwell gmaxwell added this to the 0.12.0 milestone Nov 5, 2015

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 6, 2015

Member

Concept ACK. However in my first attempt at benchmarking I'm not seeing any speedup with this code change (in fact block verification times seem to have gotten very, very slightly slower for some reason). Will continue to debug and try to see if my benchmarking is broken, or if there's some effect here we're missing.

Member

sdaftuar commented Nov 6, 2015

Concept ACK. However in my first attempt at benchmarking I'm not seeing any speedup with this code change (in fact block verification times seem to have gotten very, very slightly slower for some reason). Will continue to debug and try to see if my benchmarking is broken, or if there's some effect here we're missing.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 11, 2015

Member

@sdaftuar The results #6976 indicate that this improves things. Does that contradict your earlier findings?

Member

sipa commented Nov 11, 2015

@sdaftuar The results #6976 indicate that this improves things. Does that contradict your earlier findings?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 11, 2015

Member

Sorry forgot to update with my results. I ran longer studies comparing the old and new behavior more carefully and yes I agree this does improve things. I saw roughly a 5% speed up in processing all the October historical data using the new default cache size compared with the old implementation (running with a sig cache of approximately the same size).

ACK

Member

sdaftuar commented Nov 11, 2015

Sorry forgot to update with my results. I ran longer studies comparing the old and new behavior more carefully and yes I agree this does improve things. I saw roughly a 5% speed up in processing all the October historical data using the new default cache size compared with the old implementation (running with a sig cache of approximately the same size).

ACK

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 11, 2015

Member

ACK

Member

gmaxwell commented Nov 11, 2015

ACK

@laanwj laanwj merged commit 69d373f into bitcoin:master Nov 12, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Nov 12, 2015

Merge pull request #6918
69d373f Don't wipe the sigcache in TestBlockValidity (Pieter Wuille)
0b9e9dc Evict sigcache entries that are seen in a block (Pieter Wuille)
830e3f3 Make sigcache faster and more efficient (Pieter Wuille)

zkbot added a commit to zcash/zcash that referenced this pull request May 14, 2018

Auto merge of #3262 - str4d:2074-perf-1, r=<try>
Bitcoin 0.12 performance improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6918
- bitcoin/bitcoin#6932

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request May 14, 2018

Auto merge of #3262 - str4d:2074-perf-1, r=<try>
Bitcoin 0.12 performance improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6918
- bitcoin/bitcoin#6932

Part of #2074.

zkbot added a commit to zcash/zcash that referenced this pull request May 31, 2018

Auto merge of #3262 - str4d:2074-perf-1, r=<try>
Bitcoin 0.12 performance improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6918
- bitcoin/bitcoin#6932

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