Expire dedups on tick
#297
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the-mikedavis
commented
Sep 23, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 89.69% 90.25% +0.56%
==========================================
Files 22 22
Lines 3259 3642 +383
==========================================
+ Hits 2923 3287 +364
- Misses 336 355 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dumbbell
reviewed
Sep 24, 2024
|
I think it should be easier to start with the other optimization (which also must bump the machine version) and this PR should depend on rabbitmq/ra#474 so I'll mark this as a draft for now. We should be able to come back to it after the other PR is merged. |
Ra 2.10.1 introduced a new `handle_aux/5` callback that takes the place of `handle_aux/6`. Instead of passing the log state and machine state separately, this API passes a new `ra_aux:internal_state()` opaque argument which you can read log or machine state out of with helper functions from the `ra_aux` module. This commit only updates to use the new callback: there should be no functional change from this commit.
This will be used in the child commit to write a test which sets a low `tick_timeout` configuration.
the-mikedavis
force-pushed
the
md/expire-dedups-on-tick
branch
from
73a6492
to
abc3ebe
Compare
the-mikedavis
force-pushed
the
md/expire-dedups-on-tick
branch
from
abc3ebe
to
a106501
Compare
dumbbell
requested changes
Oct 1, 2024
As part of the `khepri_machine:post_apply/2` helper which is run after
any command is applied, we use `maps:filter/2` to eliminate any entries
in the `dedups` field of the machine state which are expired according
to the command's timestamp. This `drop_expired_dedups/2` step becomes a
bottleneck though when a Khepri store handles many transactions at once.
For example in RabbitMQ, queue deletion is done with a transaction
submitted from each queue process. When many (for example five thousand)
queues are deleted at once, `drop_expired_dedups/2` becomes a noticeable
chunk of a flamegraph and specifically the `maps:filter/2` within.
`maps:filter/2` is slow here because the BIF used to implement it
collects a list of all key-value pairs for which the predicate returns
true, sorts it, and then creates a new hashmap from it. We are unlikely
to expire any given dedup when handling a command, especially when
submitting many commands at once, so we end up essentially calling
`maps:from_list(maps:to_list(Dedups))`.
It is a small improvement to replace `maps:filter/2` with `maps:fold/3`,
a case expression and `maps:remove/2` (reflecting that we will always
fold over the map but rarely remove elements) but it is not enough to
eliminate this rather large chunk of the flamegraph.
Instead the solution in this commit is to move the detection of expired
dedups to the "tick" aux effect. Ra emits this effect periodically:
every 1s by default but configurable. By moving this detection to
`handle_aux/5` we remove it from the "hot path" of `apply/3`. Even
better though: we are unlikely to actually need to expire any dedups.
In the case of queue deletion in RabbitMQ for example, we are likely
to handle all in-flight transactions and handle the subsequent
`#dedup_ack{}` commands before even evaluating a `tick` effect. If we
do handle a tick effect while transactions are in-flight then we are
unlikely to need to expire them anyways so we will only scan the map
with the new `khepri_utils:maps_any/2` helper.
If we need to expire dedups, the aux handler for the `tick` effect
appends a new `#expire_dedups{}` command to the log which does the same
as `drop_expired_dedups/2` prior to this commit.
the-mikedavis
force-pushed
the
md/expire-dedups-on-tick
branch
from
a106501
to
504ffd9
Compare
dumbbell
approved these changes
Oct 2, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change moves the work we do in
khepri_machine:drop_expired_dedups/2out ofapply/3and into the handler for Ra's periodictickaux effect.The commit message has more details but the TLDR is that the
maps:filter/2withindrop_expired_dedups/2can become expensive if many transactions are being handled before their dedups are received. Instead we can determine if any dedups are expired in the handler for thetickeffect and expire any by committing a new#expire_dedups{}command.Also included is a change to use the new
handle_aux/5API introduced in Ra 2.10 but that change shouldn't have any effect on behavior.