feat: Make MonitorUpdatingPersister change persist type based on size #3834
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
| /// For small channel monitors (below `minimum_monitor_size_for_updates` bytes when serialized), | ||
| /// this persister will always write the full monitor instead of individual updates. This avoids | ||
| /// the overhead of managing update files and later compaction for tiny monitors that don't benefit | ||
| /// from differential updates. |
There was a problem hiding this comment.
It is still not clear to me how much the gain is of this in practice. Also worried that disabling the incremental path initially allow certain bugs to linger for longer, just because the path isn't hit as much, or rarely.
Prabhat1308
force-pushed
the
probot/change_persist_type
branch
from
ac6966e to
c6ad41b
Compare
There was a problem hiding this comment.
Please rewrite your commit history so that it's clear what are feature and fixup commits. Each commit message should have a clear headline followed by some paragraph(s) describing the change, where it happened, and why it was necessary. For guidance, please take a look at https://cbea.ms/git-commit/
Introduces an optimization to the MonitorUpdatingPersister to avoid writing differential updates for small channel monitors. When a channel monitor is smaller than a configurable threshold , the persister will now write the full monitor instead of an update. This avoids the I/O overhead of creating and managing many small update files for monitors that don't benefit significantly from differential updates.
Adds unit test for introduced size based optimisation. Also updates the old unit test to use a threshold value and use constructors to increase test coverage
Prabhat1308
force-pushed
the
probot/change_persist_type
branch
from
357ce4b to
b313e39
Compare
|
I have created a minimal benchmarking setup here for this. from this I get that adding update based persistence is reducing the performance completely opposite to what is claimed in the issue . [There are a lot of assumptions in the benchmarking regarding the update size and monitor sizes ] |
fixes #3770
minimum_monitor_size_for_updatesto specify the minimum size for full persistence to be activatednew_with_default_thresholdfunction to setupMonitorUpdatingPersisterwith a defaultminimum_monitor_size_for_updatesvalue