feat(twap): prune records over multiple blocks #7427
Conversation
| // pruneRecords prunes twap records that happened earlier than recordHistoryKeepPeriod | ||
| // before current block time while preserving the most recent record before the threshold. | ||
| // Such record is preserved for each pool. | ||
| // See TWAP keeper's `pruneRecordsBeforeTimeButNewest(...)` for more details about the reasons for | ||
| // keeping this record. | ||
| func (k Keeper) pruneRecords(ctx sdk.Context) error { | ||
| recordHistoryKeepPeriod := k.RecordHistoryKeepPeriod(ctx) | ||
|
|
||
| lastKeptTime := ctx.BlockTime().Add(-recordHistoryKeepPeriod) | ||
| return k.pruneRecordsBeforeTimeButNewest(ctx, lastKeptTime) | ||
| } |
There was a problem hiding this comment.
This helper func is no longer needed since we determine the lastKeptTime at epoch end and store in a state entry.
There was a problem hiding this comment.
Seems like we should still keep this function so we can unit test it? (Just move the logic into here)
There was a problem hiding this comment.
Or do you think the current test is good / simple enough?
There was a problem hiding this comment.
I can reimplement the test, I thought the state.go test tests the same thing, but I can double check
There was a problem hiding this comment.
Yeah this is tested in even more depth here
Lines 321 to 555 in 81e7ebe
Deleting the func along with the test is fine imo
|
Cool! Looks like as is this is shaving 25 seconds off of epoch. (Including the TWAP CPU time) I'd guess that a lot of this 7 seconds is still from prunes in next 2 blocks. Per your data in slack, epoch is deleting ~12k keys (assuming I counted correctly) that are not TWAP, so thats probably a lot of the 7 seconds then.
But half the TWAP deletes are very "data local", so will have less of branchy-ness. The other half should be fairly data local, but won't be if its split up. So right now were deleting 2k/block in the above benchmark. (Applying my above comment makes this 1k/block) |
czarcas7ic
requested review from
nicolaslara,
p0mvn and
AlpinYukseloglu
as code owners
| // We have not pruned all records, so we update the last key seen. | ||
| state.LastKeySeen = iter.Key() | ||
| k.SetPruningState(ctx, state) |
There was a problem hiding this comment.
I may ahve missed it, but I don't think we have any logic testing this code path. (or rather that keys from this are working as we want)
From reading the code, this loosk right to me!
There was a problem hiding this comment.
We don't, I will add before merge
There was a problem hiding this comment.
This test shows pruning happening over multiple blocks f84a716
It verifies
- We see the extra record that we save due to the bug
- Pruning eventually gets set to false when we iterate over records
- The correct records get pruned
* twap prune over multiple blocks * changelog entry * fix after epoch end test * track where we left off in iteration * add comment to explain better * Generated protofile changes * inc by 2 * inc by 2 * add comments * add twap pruning test over multiple blocks --------- Co-authored-by: github-actions <github-actions@github.com> (cherry picked from commit 1c9e491) # Conflicts: # CHANGELOG.md
* twap prune over multiple blocks * changelog entry * fix after epoch end test * track where we left off in iteration * add comment to explain better * Generated protofile changes * inc by 2 * inc by 2 * add comments * add twap pruning test over multiple blocks --------- Co-authored-by: github-actions <github-actions@github.com> (cherry picked from commit 1c9e491) # Conflicts: # CHANGELOG.md Co-authored-by: Adam Tucker <adam@osmosis.team>
| // Two records are deleted per incentive record: | ||
| // 1. by time index | ||
| // 2. by pool index | ||
| // Therefore, setting this to 1000 means 500 complete incentive records are deleted per block. |
| // Two records are deleted per incentive record: | ||
| // 1. by time index | ||
| // 2. by pool index | ||
| // Therefore, setting this to 1000 means 500 complete incentive records are deleted per block. |
There was a problem hiding this comment.
Can we elaborate on the choice here? Why 1000/500?
Closes: #XXX
What is the purpose of the change
As shown by the above pprof, we spend almost 30s of epoch deleting entries. When adding print lines, we saw that a majority of these deletes are coming from the TWAP module. This can safely be limited and done over multiple blocks instead of being done at time of epoch.
This PR hardcodes a limit of 1000 entries per block. We also do the pruning at endBlock instead of afterEpochEnd.
Testing and Verifying
Profile shows reduction in delete time from 30s to 7s
Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)