Txhandler: Improve dedupe hit rate #5022
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5022 +/- ##
=======================================
Coverage 53.63% 53.64%
=======================================
Files 432 432
Lines 54057 54060 +3
=======================================
+ Hits 28996 28999 +3
- Misses 22812 22816 +4
+ Partials 2249 2245 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
iansuvak
changed the title
data/txDupCache.go
Outdated
| if _, found := c.cur[*d]; found { | ||
| return d, true | ||
| } | ||
| if _, found := c.prev[*d]; found { |
There was a problem hiding this comment.
the prev map is using a different salt. I think you would need to have innerCheck return (d *crypto.Digest, pd *crypto.Digest, hit bool) instead of just the one digest for cur to avoid rehash here.
|
I guess would need to step back and look at how often these rare duplicates are getting through in practice, and is it more performant overall to let that small fraction through than to pay a penalty of what looks like 4-8% slower performance overall from your benchmarks? |
|
11.7% slower for the |
|
It's about 2.5-3% of the transactions that we see are duplicates and could reasonably only check the cur to avoid having to rehash with the old salt. Benchmarks for that are here which looks like 4% slowdown on the thread=16 case. The real cost to weigh is the cost of this slowdown vs the cost of re-processing these through the TxHandler right? : |
|
That's right, the cost of the slowdown for all cache checks vs the reprocessing cost of the ones that got through. Do you know how many total transactions were processed during the period you were looking at? Not just the ones that made it through the filter but the raw TX message count that hit this code — that would push the duplicate rate you're seeing of 2.5%-3% a lot further down I assume? (Also these changes are only affecting saltedCacheMaker not the digestCacheMaker so the benchmarks are only relevant for half the results you're posting) |
The TX messages that didn't make it through the filter by definition wouldn't hit this code no? They got filtered out using RLocks only right? The raw number is a lot larger of course but it still seems to me that this would be worth it iff: 2.5% * thandleTxn >= ~120ns Do we know the average or worst case t above?
Yup, Just posted the whole output of the single Benchmark function without editing it. |
|
your node should have been constantly increasing its tx counter during this time, so you should have access to the value of the tx counter value was before and after your data collection |
Summary
Add a check in the cache without rehashing before writing and returning to avoid the race condition that allows close to simultaneous received transactions to miss the cache due to a race condition.
Currently checks both caches but could reasonably only check the current cache if we wanted to trade some time cost for slightly lower hit rate.
Test Plan
Existing tests should pass.
Benchmark results:
Old:
New: