Merge in btcd commit 6229e3583505a82d4514b1efa86f910b78693825 #381
Conversation
This commit drastically reduces the number of allocations needed to deserialize a transaction and its scripts by using the combination of a free list for initially deserializing the individual scripts along with copying them into a single contiguous byte slice after the final size is known and modifying each script in the transaction to point to its location within the contiguous blob. The end result is only a single allocation that holds all of the scripts for a transaction regardless of the total number of scripts it has. The script free list allows a maximum of 12,500 items with each buffer being 512 bytes. This implies it will have a peak usage of 6.1MB. The values were chosen based on profiling data and a desire to allow at least 100 scripts per transaction to be simultaneously deserialized by 125 peers. Also, while optimizing, decode directly into the existing previous outpoint structure of each transaction input in order to avoid the extra allocation per input that is otherwise caused when the local escapes to the heap. The following is a before and after comparison of the allocations with the benchmarks that did not change removed: benchmark old allocs new allocs delta ----------------------------------------------------------- ReadTxOut 1 0 -100.00% ReadTxIn 2 0 -100.00% DeserializeTxSmall 7 5 -28.57% DeserializeTxLarge 11146 6 -99.95%
| // because it is slightly more than twice the size of the vast majority | ||
| // of all "standard" scripts. Larger scripts are still deserialized | ||
| // properly as the free list will simply be bypassed for them. | ||
| freeListMaxScriptSize = 512 |
There was a problem hiding this comment.
Might want to consider tweaking this number since I don't believe the standard Decred transaction script size is the same as Bitcoin.
There was a problem hiding this comment.
They're very similar in size. I'm not sure it's required that they be tweaked, at least, not at the moment.
| // scripts per transaction being simultaneously deserialized by 125 | ||
| // peers. Thus, the peak usage of the free list is 12,500 * 512 = | ||
| // 6,400,000 bytes. | ||
| freeListMaxItems = 12500 |
There was a problem hiding this comment.
And of course if the feeListMaxScriptSize is tweaked, this and/or the comment would need to be updated too.
| // that would otherwise be needed to keep track of millions of small | ||
| // allocations. | ||
| // | ||
| // NOTE: It is no longer valid to call the returnScriptBuffers closure |
There was a problem hiding this comment.
This part of the comment should be on the function definition now since it was split out and there is no longer any such function accessible in this scope.
Also, since it's not in scope even at the function level, the comment should be modified accordingly. Perhaps something like NOTE: It is no longer valid to return any previously borrowed script buffers after ...
| // writeTxInScriptsToMsgTx allocates the memory for variable length fields in a | ||
| // MsgTx TxIns as a contiguous chunk of memory, then fills in these fields for the | ||
| // MsgTx by copying to a contiguous piece of memory and setting the pointer. | ||
| func writeTxInScriptsToMsgTx(msg *MsgTx, totalScriptSize uint64) { |
There was a problem hiding this comment.
Splitting these reduces the effectiveness since it will end up with two allocations instead of one single allocation. I would create a single func like writeTxScriptsToMsgTx or even makeScriptsContiguous(msg *MsgTx, totalScriptSize uint64)
| switch { | ||
| case mType == TxSerializeNoWitness: | ||
| err := msg.decodePrefix(r, pver) | ||
| err := msg.decodePrefix(r, pver, returnScriptBuffers) |
There was a problem hiding this comment.
Rather than passing the returnScriptBuffers func to all of these and calling it in their error paths, I propose just calling it from this function in the if err != nil { blocks. That way the called funcs can't forget to invoke it since they aren't responsible for it.
As a case in point, the decodeWitnessSigning function never returns the buffers on error.
| @@ -701,11 +898,13 @@ func (msg *MsgTx) decodeWitness(r io.Reader, pver uint32, isFull bool) error { | |||
| } | |||
| } | |||
|
|
|||
| writeTxInScriptsToMsgTx(msg, totalScriptSize) | |||
There was a problem hiding this comment.
I think this needs a comment to explain what's going on since this would be quite unintuitive for a reader that is not already familiar with this code.
| } | ||
| msg.TxOut = make([]*TxOut, 0) | ||
|
|
||
| writeTxOutScriptsToMsgTx(msg, totalScriptSize) |
There was a problem hiding this comment.
I think this needs a comment to explain what's going on since this would be quite unintuitive for a reader that is not already familiar with this code.
| return err | ||
| } | ||
|
|
||
| writeTxOutScriptsToMsgTx(msg, totalScriptSize) |
There was a problem hiding this comment.
I think this needs a comment to explain what's going on since this would be quite unintuitive for a reader that is not already familiar with this code.
| return err | ||
| } | ||
|
|
||
| writeTxInScriptsToMsgTx(msg, totalScriptSize) |
There was a problem hiding this comment.
I think these need a comment to explain what's going on since this would be quite unintuitive for a reader that is not already familiar with this code.
cjepson
force-pushed
the
cj_230916sync6229
branch
from
810276f
to
d198d4c
Compare
|
Cleaned up a lot. The writes have been merged into one, and the closure is no longer passed through the functions. Should be easier to read. |
Merges in btcd commit 6229e35.
cjepson
force-pushed
the
cj_230916sync6229
branch
from
d198d4c
to
615c018
Compare
Merges in btcd commit 6229e35.