osd_types: optimized version of pg_pool_t::build_removed_snaps #17493
Conversation
|
Jenkins retest this please |
|
@joscollin can you add a performance tag too? This reduces cpu usage spikes on OSDMap updates when there's a lot of snapshots. |
branch-predictor
force-pushed
the
bp-optimized-brs
branch
from
063f3f7
to
7409841
Compare
Previous version interated over entire range 1..get_snap_seq, and each time over entire snaps map (which is lg*snaps.size, making it M*lgN), adding each missing snap by one (up to M*lgN). Reduce that considerably by iterating only over existing snaps and adding missing ones as ranges instead of relying on interval_set.insert(el, 1) to coalesce them on-fly. Signed-off-by: Piotr Dałek <piotr.dalek@corp.ovh.com>
This introduces new method (build_removed_snaps_and_check_intersect) which checks whether cached removed snaps interval set intersects newly build removed snaps interval set. This lets us get rid of creation of intermediate interval set, which is expensive in terms of both CPU and memory when there's a lot of snaps. Signed-off-by: Piotr Dałek <piotr.dalek@corp.ovh.com>
branch-predictor
force-pushed
the
bp-optimized-brs
branch
from
7409841
to
cb091d3
Compare
| // this iterates linearly over snaps map, building deleted snap intervals in rs. | ||
| // it compares that with cached removed snaps (cached) and returns false if any of | ||
| // interval elements in cached doesn't exists in newly removed snaps (rs). | ||
| bool pg_pool_t::build_removed_snaps_and_check_intersect(interval_set<snapid_t>& rs, |
There was a problem hiding this comment.
It looks like the code in this method is only valid when is_pool_snaps_mode() returns true, so while it may be valid if you're using pool snaps, it doesn't seem valid if you're using self managed snaps.
There was a problem hiding this comment.
Since cached is not modified in this method, it can be passed as a const reference.
There was a problem hiding this comment.
I suspect this whole method is equivalent to the following:
build_removed_snaps(rs);
return cached.subset_of(rs);There was a problem hiding this comment.
I've distilled your patch into the following:
There was a problem hiding this comment.
In order for subset_of to be viable, it would need to dynamically choose between sequential search and lower_bound methods, like the intersect_of implementation since #17088.
|
As @zmedico suggests, this only works if using pool snapshots — which I expect to be almost nobody. The optimizations for pool snapshots in build_removed_snaps looked right to me, so we can merge them in separately. But I’m closing the PR for the rest of it. :) |
Previous version iterated over entire range 1..get_snap_seq, and each time over entire snaps map (which is lgsnaps.size, making it MlgN), adding each missing snap by one (up to M*lgN). Reduce that considerably by iterating only over existing snaps and adding missing ones as ranges instead of relying on interval_set.insert(el) to coalesce them on-fly.
Signed-off-by: Piotr Dałek piotr.dalek@corp.ovh.com