table: Fix quadratic behavior when inserting sstables into tracker on schema change #12593

raphaelsc · 2023-01-21T02:19:57Z

Each time backlog tracker is informed about a new or old sstable, it will recompute the static part of backlog which complexity is proportional to the total number of sstables.
On schema change, we're calling backlog_tracker::replace_sstables() for each existing sstable, therefore it produces O(N ^ 2) complexity.

Fixes #12499.

Signed-off-by: Raphael S. Carvalho raphaelsc@scylladb.com

scylladb-promoter · 2023-01-21T06:35:16Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3944/

bhalevy · 2023-01-23T12:34:48Z

replica/table.cc

            });
        }

        void execute() noexcept {
+            new_bt.replace_sstables({}, std::move(new_sstables_for_backlog_tracker));


new_sstables_for_backlog_tracker can also be computed on-the-fly here using boost::copy_range<sstables::shared_sstable>(*new_sstables->all), no?
It might be slightly more efficient since it will reserve the required capacity once.

This is noexcept path though. also not a hot path. I can reserve new_sstables_for_backlog_tracker upfront. what do you think?

This is noexcept path though.

ok, so let's do that in prepare.

also not a hot path. I can reserve new_sstables_for_backlog_tracker upfront. what do you think?

Do we know the size upfront?
I don't think sstable_set gives you that, does it?

This is noexcept path though.

ok, so let's do that in prepare.

also not a hot path. I can reserve new_sstables_for_backlog_tracker upfront. what do you think?

Do we know the size upfront? I don't think sstable_set gives you that, does it?

I think we can easily implement a size() method for sstable_set. Today, we have to call all() which can copy elements, but that's possibly better than reallocating log base_2 N, where N is number of elements inserted into container.

compaction_backlog_tracker::replace_sstables() can fail, no?

This is noexcept path though.

ok, so let's do that in prepare.

also not a hot path. I can reserve new_sstables_for_backlog_tracker upfront. what do you think?

Do we know the size upfront? I don't think sstable_set gives you that, does it?

I think we can easily implement a size() method for sstable_set. Today, we have to call all() which can copy elements, but that's possibly better than reallocating log base_2 N, where N is number of elements inserted into container.

Let's consider this for follow up then.

compaction_backlog_tracker::replace_sstables() can fail, no?

Yup,

scylladb/compaction/compaction_manager.cc

Line 1725 in 5417074

ret.push_back(sst);

Why does execute need to be noexcept?

Sorry, technically errors are handled their in

scylladb/compaction/compaction_manager.cc

Lines 1731 to 1737 in 5417074

try {

_impl->replace_sstables(filter_and_revert_charges(old_ssts), filter_and_revert_charges(new_ssts));

} catch (...) {

cmlog.error("Disabling backlog tracker due to exception {}", std::current_exception());

// FIXME: tracker should be able to recover from a failure, e.g. OOM, by having its state reset. More details on https://github.com/scylladb/scylla/issues/10297.

disable();

}

But I'm not sure if that's the best course of action.
Seems harsh to disable the backlog tracker on e.g. transient memory pressure.

This is noexcept path though.

ok, so let's do that in prepare.

also not a hot path. I can reserve new_sstables_for_backlog_tracker upfront. what do you think?

Do we know the size upfront? I don't think sstable_set gives you that, does it?

I think we can easily implement a size() method for sstable_set. Today, we have to call all() which can copy elements, but that's possibly better than reallocating log base_2 N, where N is number of elements inserted into container.

Let's consider this for follow up then.

sure.

raphaelsc · 2023-01-24T14:47:19Z

v2: replace sstables in new bt in prepare() phase, then execute() remains noexcept.

… schema change Each time backlog tracker is informed about a new or old sstable, it will recompute the static part of backlog which complexity is proportional to the total number of sstables. On schema change, we're calling backlog_tracker::replace_sstables() for each existing sstable, therefore it produces O(N ^ 2) complexity. Fixes scylladb#12499. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>

bhalevy · 2023-01-24T16:25:42Z

replica/table.cc

-            cg.main_sstables()->for_each_sstable([this] (const sstables::shared_sstable& s) {
-                add_sstable_to_backlog_tracker(new_bt, s);
+            std::vector<sstables::shared_sstable> new_sstables_for_backlog_tracker;
+            new_sstables_for_backlog_tracker.reserve(cg.main_sstables()->all()->size());


Doesn't all build a sstable_list?
If so, we can just keep it and use it here?

Not always. It builds a new list if the set is compound (maintenance + main or sets of all groups). Here we're working on main set only of a single group.

For partititioned_sstable_set, for example, we just return its local copy of all.

I think the interface is fragile as user can modify local copy if it doesn't copy on write.

lw_shared_ptr<sstable_list> all() const;

should be changed to

lw_shared_ptr<const sstable_list> all() const;

scylladb-promoter · 2023-01-24T18:42:36Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3996/

scylladb-promoter · 2023-01-24T19:06:00Z

CI state SUCCESS - https://jenkins.scylladb.com/job/releng/job/Scylla-CI/3997/

raphaelsc · 2023-01-25T17:04:33Z

@scylladb/scylla-maint ping.

… schema change Each time backlog tracker is informed about a new or old sstable, it will recompute the static part of backlog which complexity is proportional to the total number of sstables. On schema change, we're calling backlog_tracker::replace_sstables() for each existing sstable, therefore it produces O(N ^ 2) complexity. Fixes scylladb#12499. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes scylladb#12593

… schema change Each time backlog tracker is informed about a new or old sstable, it will recompute the static part of backlog which complexity is proportional to the total number of sstables. On schema change, we're calling backlog_tracker::replace_sstables() for each existing sstable, therefore it produces O(N ^ 2) complexity. Fixes scylladb#12499. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes scylladb#12593 (cherry picked from commit 87ee547) Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>

raphaelsc requested review from denesb and bhalevy January 23, 2023 00:11

bhalevy reviewed Jan 23, 2023

View reviewed changes

denesb approved these changes Jan 24, 2023

View reviewed changes

bhalevy approved these changes Jan 24, 2023

View reviewed changes

raphaelsc force-pushed the fix-quadratic-behavior-backlog-tracker-on-schema-change branch from f57c507 to 7149404 Compare January 24, 2023 14:46

raphaelsc requested a review from avikivity January 24, 2023 14:50

raphaelsc force-pushed the fix-quadratic-behavior-backlog-tracker-on-schema-change branch from 7149404 to ed7dbfe Compare January 24, 2023 14:53

bhalevy reviewed Jan 24, 2023

View reviewed changes

scylladb-promoter closed this in 87ee547 Jan 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

table: Fix quadratic behavior when inserting sstables into tracker on schema change #12593

table: Fix quadratic behavior when inserting sstables into tracker on schema change #12593

raphaelsc commented Jan 21, 2023

scylladb-promoter commented Jan 21, 2023

bhalevy Jan 23, 2023

raphaelsc Jan 23, 2023

bhalevy Jan 24, 2023

raphaelsc Jan 24, 2023

avikivity Jan 24, 2023

bhalevy Jan 24, 2023

bhalevy Jan 24, 2023

bhalevy Jan 24, 2023

bhalevy Jan 24, 2023

raphaelsc Jan 24, 2023

raphaelsc commented Jan 24, 2023

bhalevy Jan 24, 2023

raphaelsc Jan 24, 2023 •

edited

scylladb-promoter commented Jan 24, 2023

scylladb-promoter commented Jan 24, 2023

raphaelsc commented Jan 25, 2023

	try {
	_impl->replace_sstables(filter_and_revert_charges(old_ssts), filter_and_revert_charges(new_ssts));
	} catch (...) {
	cmlog.error("Disabling backlog tracker due to exception {}", std::current_exception());
	// FIXME: tracker should be able to recover from a failure, e.g. OOM, by having its state reset. More details on https://github.com/scylladb/scylla/issues/10297.
	disable();
	}

table: Fix quadratic behavior when inserting sstables into tracker on schema change #12593

table: Fix quadratic behavior when inserting sstables into tracker on schema change #12593

Conversation

raphaelsc commented Jan 21, 2023

scylladb-promoter commented Jan 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelsc commented Jan 24, 2023

Choose a reason for hiding this comment

raphaelsc Jan 24, 2023 • edited

Choose a reason for hiding this comment

scylladb-promoter commented Jan 24, 2023

scylladb-promoter commented Jan 24, 2023

raphaelsc commented Jan 25, 2023

raphaelsc Jan 24, 2023 •

edited