txHandler: make dedup working set independent from ERL #5200
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5200 +/- ##
==========================================
- Coverage 53.57% 53.56% -0.01%
==========================================
Files 441 441
Lines 55094 55098 +4
==========================================
- Hits 29515 29513 -2
+ Misses 23300 23296 -4
- Partials 2279 2289 +10
... and 10 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| cacheConfig: txHandlerConfig{opts.Config.TxFilterRawMsgEnabled(), opts.Config.TxFilterCanonicalEnabled()}, | ||
| streamVerifierChan: make(chan execpool.InputJob), | ||
| streamVerifierDropped: make(chan *verify.UnverifiedTxnSigJob), | ||
| } | ||
|
|
||
| // use defaultBacklogSize = approx number of txns in a full block as a parameter for the dedup cache size | ||
| defaultBacklogSize := config.GetDefaultLocal().TxBacklogSize |
There was a problem hiding this comment.
Why config.GetDefaultLocal().TxBacklogSize and not opts.Config.TxBacklogSize?
This function is now juggling with 3 different backlog size concepts:
- defaultBacklogSize := config.GetDefaultLocal().TxBacklogSize
- defaultBacklogSize := opts.Config.TxBacklogSize
- txBacklogSize += (opts.Config.IncomingConnectionsLimit * opts.Config.TxBacklogReservedCapacityPerPeer)
Although (2) is not used in that form, but used as (3), (1) and (3) may or may not always originate from the same value.
There was a problem hiding this comment.
config.GetDefaultLocal().TxBacklogSize returns real default and opts.Config.TxBacklogSize returns actual value and they could be different (that's where the issue comes from)
| // use defaultBacklogSize = approx number of txns in a full block as a parameter for the dedup cache size | ||
| defaultBacklogSize := config.GetDefaultLocal().TxBacklogSize | ||
| if handler.cacheConfig.enableFilteringRawMsg { | ||
| handler.msgCache = makeSaltedCache(2 * defaultBacklogSize) |
There was a problem hiding this comment.
It was understandable to have the size of msgCache and txCanonicalCache to be the same as backlogQueue size.
After all, no regression was observed when the cache sizes were set to the queue sizes.
But now, ERL changes the queue and cache sizes, we get regression, and the fix is reverting the cache sizes but not the queue sizes.
What is the justification for the ERL imposed size, and consequently, the separation of the cache sizes from the queue sizes?
There was a problem hiding this comment.
decreasing sets size triggers rotation faster => higher chances pass a dup msg seen say 10 seconds ago
algorandskiy
force-pushed
the
pavel/dedup-fix
branch
from
fdc6c48
to
31cf4b8
Compare
There was a problem hiding this comment.
This PR is addressing a fallout from a different parameter change. This PR is fine.
I am worried that too many parameters are controlling the critical pipeline of txn verification, and this will have adverse effects on the overall performance. This is out of the scope of this PR.
|
Thank you folks for looking into this. |
Summary
Fine tuning ERL might affect the dedup working set size in a negative way. This PR fixes that.
Additionally, make dedup creation optional (only if enabled). This is not important but looks a right thing to do.
Plus couple non-related small comment/assert noted along the way.
Test Plan
Existing tests