Skip to content
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

reduce unnecessary locking and copying; increase peer maxKnownTxs #39

Merged
merged 1 commit into from
Mar 6, 2018

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Mar 6, 2018

This change removes or replaces some uses of a generic concurrent set from gopkg.in/fatih/set.v0, removes some unnecessary slice copies, and increases maxKnownTxs.

@@ -178,7 +177,7 @@ func (ethash *Ethash) VerifyUncles(chain consensus.ChainReader, block *types.Blo
return errTooManyUncles
}
// Gather the set of past uncles and ancestors
uncles, ancestors := set.New(), make(map[common.Hash]*types.Header)
uncles, ancestors := make(map[common.Hash]struct{}), make(map[common.Hash]*types.Header)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to lock in here.

@@ -229,7 +229,7 @@ func GetBlock(db DatabaseReader, hash common.Hash, number uint64) *types.Block {
return nil
}
// Reassemble the block and return
return types.NewBlockWithHeader(header).WithBody(body.Transactions, body.Uncles)
return types.NewBlockWith(header, body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We just deserialized these, so nobody else has them, don't call routines that make copies, use a new function to just set them.

@@ -705,7 +705,7 @@ func (pm *ProtocolManager) BroadcastBlock(block *types.Block, propagate bool) {
// Otherwise if the block is indeed in out own chain, announce it
if pm.blockchain.HasBlock(hash, block.NumberU64()) {
for _, peer := range peers {
peer.SendNewBlockHashes([]common.Hash{hash}, []uint64{block.NumberU64()})
peer.SendNewBlockHash(hash, block.NumberU64())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only usage. Don't make slices.

@@ -37,8 +36,8 @@ var (
)

const (
maxKnownTxs = 32768 // Maximum transactions hashes to keep in the known list (prevent DOS)
maxKnownBlocks = 1024 // Maximum block hashes to keep in the known list (prevent DOS)
maxKnownTxs = 262144 // Maximum transactions hashes to keep in the known list (prevent DOS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per peer set of known txs. Octupled. Maybe overkill?

type setOfHash struct {
sync.RWMutex
m map[common.Hash]struct{}
}
Copy link
Contributor Author

@jmank88 jmank88 Mar 6, 2018

Choose a reason for hiding this comment

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

Minimal custom replacement for gopkg.in/fatih/set.v0.Set. Besides avoiding interface{} conversions, we also can do smarter locking, e.g. locking once outside a big loop instead of repeatedly inside the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Named awkwardly b/c I thought hashSet or commonHashSet would be confusing.

}
r.knownTxs.m = make(map[common.Hash]struct{}, maxKnownTxs)
r.knownBlocks.m = make(map[common.Hash]struct{}, maxKnownBlocks)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make these at cap to begin with.

ancestors map[common.Hash]struct{} // ancestor set (used for checking uncle parent validity)
family map[common.Hash]struct{} // family set (used for checking uncle invalidity)
uncles map[common.Hash]struct{} // uncle set
uncleMu sync.RWMutex
Copy link
Contributor Author

@jmank88 jmank88 Mar 6, 2018

Choose a reason for hiding this comment

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

Only uncles needs to lock. ancestors and family are only read after being set up (from makeCurrent()).

@treeder treeder merged commit a0b0dff into master Mar 6, 2018
@jmank88 jmank88 deleted the less-locks branch March 21, 2018 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants