blockchain: Use slices when fetching utxos #1972
Conversation
Pull Request Test Coverage Report for Build 4894156905
💛 - Coveralls |
blockchain/utxoviewpoint.go
Outdated
| @@ -517,7 +517,7 @@ func (view *UtxoViewpoint) fetchUtxosMain(db database.DB, outpoints map[wire.Out | |||
| // so other code can use the presence of an entry in the store as a way | |||
| // to unnecessarily avoid attempting to reload it from the database. | |||
| return db.View(func(dbTx database.Tx) error { | |||
| for outpoint := range outpoints { | |||
| for _, outpoint := range outpoints { | |||
There was a problem hiding this comment.
Should we use index based access to the slice to avoid any Golang loop variable issues?
There was a problem hiding this comment.
I can change it to index based access.
Could you let me know what the Golang loop variable issues are? I'm not aware of these issues so it'd be good to know.
There was a problem hiding this comment.
The variable is defined per-loop, not per-iteration. So depending on how it is used, it can lead to unexpected side effects. See https://mohamed-abdel-mohaimen.medium.com/using-loop-iterator-variable-issues-in-golang-b76c17fb71b6 or similar articles.
blockchain/utxoviewpoint.go
Outdated
| @@ -626,23 +626,23 @@ func (b *BlockChain) FetchUtxoView(tx *btcutil.Tx) (*UtxoViewpoint, error) { | |||
| // Create a set of needed outputs based on those referenced by the | |||
| // inputs of the passed transaction and the outputs of the transaction | |||
| // itself. | |||
| neededSet := make(map[wire.OutPoint]struct{}) | |||
| needed := make([]wire.OutPoint, 0, len(tx.MsgTx().TxIn)) | |||
There was a problem hiding this comment.
We're pre-allocating for the length of TxIn but then loop over TxOut.
kcalvinalvin
force-pushed
the
utxoview-use-slices
branch
from
7bdf104 to
7d4076c
Compare
|
Addressed the reviews in the latest push |
|
Pushed an extra commit with benchmarks. I'm seeing a ~25% performance improvement. |
blockchain/utxoviewpoint.go
Outdated
| // Already loaded into the current view. | ||
| if _, ok := view.entries[outpoint]; ok { | ||
| continue | ||
| } | ||
|
|
||
| neededSet[outpoint] = struct{}{} | ||
| needed = append(needed, outpoint) |
There was a problem hiding this comment.
I think this is the classic example of where things go wrong with Golang loop variables. Should use index based access to the outpoint. See https://stackoverflow.com/questions/71437577/appending-inside-a-loop-repeats-the-last-value-in-golang
There was a problem hiding this comment.
Addressed with the latest push.
Maps have a higher overhead than slices. As slices can be used instead of maps, we avoid the overhead of making a map.
Benchmark added to compare the performance of a map vs a slice when fetching utxos. The benchmark shows roughly 25% performance improvement when using slices instead of a map.
kcalvinalvin
force-pushed
the
utxoview-use-slices
branch
from
0c5a89c to
5ede256
Compare
| neededSet := make(map[wire.OutPoint]struct{}) | ||
| for outpoint := range outpoints { | ||
| needed := make([]wire.OutPoint, 0, len(outpoints)) | ||
| for i := range outpoints { |
There was a problem hiding this comment.
nit: could do for _, outpoint := range outpoints to minimize diff
There was a problem hiding this comment.
No, I explicitly asked to use the index based access, because the append() would otherwise not work as expected because of the Golang loop variable issue (see my resolved comments above).
There was a problem hiding this comment.
I don't think that is an issue in this case, since it's a non-ref struct being used?
That being said, better safe than sorry 😀
There was a problem hiding this comment.
Hmm, you're right, in this case it's not problematic. But we've been burned by this a couple of times in the past, so I basically only use the key/index based access. Also makes it easier to review if you know the loop bug can't bite you.
Maps have a higher overhead than slices. As slices can be used instead of maps, we avoid the overhead of making a map.
This was one of the overheads I've noticed when profiling ibd for PR #1955