core: TxPool simplifications and improvements #46
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmank88
commented
Mar 7, 2018
| for _, tx := range pool.queue[addr].Flatten() { | ||
| list := pool.queue[addr] | ||
| list.txs.ensureCache() | ||
| for _, tx := range list.txs.cache { |
There was a problem hiding this comment.
Don't alloc a slice just to iterate.
jmank88
commented
Mar 7, 2018
core/tx_pool.go
Outdated
| @@ -544,10 +540,12 @@ func (pool *TxPool) local() map[common.Address]types.Transactions { | |||
| txs := make(map[common.Address]types.Transactions) | |||
| for addr := range pool.locals.accounts { | |||
| if pending := pool.pending[addr]; pending != nil { | |||
| txs[addr] = append(txs[addr], pending.Flatten()...) | |||
| pending.txs.ensureCache() | |||
| txs[addr] = append(txs[addr], pending.txs.cache...) | |||
There was a problem hiding this comment.
Don't alloc a slice just to copy.
jmank88
commented
Mar 7, 2018
| from, _ := types.Sender(pool.signer, tx) // already validated | ||
| dirty[from] = struct{}{} | ||
| } | ||
| var errs []error |
There was a problem hiding this comment.
Don't alloc. Errors should be rare.
jmank88
commented
Mar 7, 2018
| pool.promoteExecutable(addr, list) | ||
| } | ||
| pool.finishPromotion() | ||
| } |
There was a problem hiding this comment.
Separate method to directly iterate, rather than a nil special case below which allocs a slice to iterate over.
There was a problem hiding this comment.
The diff below looks messy, but it was just broken into two helper methods.
jmank88
commented
Mar 7, 2018
| @@ -1054,7 +1063,8 @@ func (pool *TxPool) promoteExecutables(accounts []common.Address) { | |||
|
|
|||
| // Drop all transactions if they are less than the overflow | |||
| if size := uint64(list.Len()); size <= drop { | |||
| for _, tx := range list.Flatten() { | |||
| list.txs.ensureCache() | |||
| for _, tx := range list.txs.cache { | |||
There was a problem hiding this comment.
Don't alloc a slice just to iterate.
jmank88
commented
Mar 7, 2018
| @@ -341,15 +341,6 @@ func (pool *TxPool) loop() { | |||
| } | |||
| } | |||
|
|
|||
| // lockedReset is a wrapper around reset to allow calling it in a thread safe | |||
| // manner. This method is only ever used in the tester! | |||
jmank88
commented
Mar 7, 2018
| defer pool.mu.RUnlock() | ||
|
|
||
| return new(big.Int).Set(pool.gasPrice) | ||
| } |
jmank88
commented
Mar 7, 2018
| Pending() map[common.Address]types.Transactions | ||
|
|
||
| // PendingList is like Pending, but only txs. | ||
| PendingList() types.Transactions |
There was a problem hiding this comment.
Some callers were just copying map values from Pending into their own slices. This helper does it directly, to avoid extra copying into an intermediate map.
jmank88
commented
Mar 7, 2018
| @@ -101,7 +101,10 @@ type txPool interface { | |||
|
|
|||
| // Pending should return pending transactions. | |||
| // The slice should be modifiable by the caller. | |||
| Pending() (map[common.Address]types.Transactions, error) | |||
| Pending() map[common.Address]types.Transactions | |||
There was a problem hiding this comment.
Both impls always returned nil error.
|
With this branch I was able to do ~400tps smoothly for 5 minutes: |
|
Attempted 500 tps for 5 minutes. Ingestion rate was ~472tps, but processing was a bit slower, at 419tps. CPU nearly maxed as well. Blocks are larger, but also slower. |
treeder
approved these changes
Mar 7, 2018
| @@ -112,7 +111,7 @@ func (journal *txJournal) insert(tx *types.Transaction) error { | |||
|
|
|||
| // rotate regenerates the transaction journal based on the current contents of | |||
| // the transaction pool. | |||
| func (journal *txJournal) rotate(all map[common.Address]types.Transactions) error { | |||
| func (journal *txJournal) rotate(acts int, all types.Transactions) error { | |||
There was a problem hiding this comment.
Why did they have the map in the first place?
There was a problem hiding this comment.
Didn't see an obvious reason. May have just wound up that way from incremental changes, or maybe it was meant to be more readable
|
By freeing up some more resources, I was able to do 5m@500tps: |
|
Aiming for 5m@550 - Ingested at 534, processed at 455. Big slow blocks. |
jmank88
commented
Mar 7, 2018
| if err := pool.journal.load(pool.AddLocal); err != nil { | ||
| if err := pool.journal.load(func(txs types.Transactions) []error { | ||
| // No need to lock since we're still setting up. | ||
| return pool.addTxsLocked(txs, !pool.config.NoLocals) |
There was a problem hiding this comment.
Don't lock during setup. Add batches instead of one at a time and promoting executables on each one.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change simplifies and improves some things surrounding
core.TxPool