compaction: avoid excessive reallocation during input list formatting #13907

raphaelsc · 2023-05-16T12:57:29Z

with off-strategy, input list size can be close to 1k, which will lead to unneeded reallocations when formatting the list for logging.

in the past, we faced stalls in this area, and excessive reallocation (log2 ~1k = ~10) may have contributed to that.

compaction/compaction.cc

raphaelsc · 2023-05-16T14:45:56Z

v2:
introduce reserve() method
switch to fmt::join() to avoid excessive copying when assembling strings together, another stall contributor. thanks @bhalevy

bhalevy

LGTM, thanks.

scylladb-promoter · 2023-05-16T17:04:12Z

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/1279/

…ting with off-strategy, input list size can be close to 1k, which will lead to unneeded reallocations when formatting the list for logging. in the past, we faced stalls in this area, and excessive reallocation (log2 ~1k = ~10) may have contributed to that. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com>

raphaelsc · 2023-05-16T17:34:25Z

v3:
switch to std::string, as sstring doesn't implement a growth factor when appending. but fmt::join() was already enough to lazily evaluate the range elements and not trigger quadratic behavior

scylladb-promoter · 2023-05-16T20:31:52Z

CI state SUCCESS - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/1281/

DoronArazii · 2023-05-28T14:34:17Z

@raphaelsc / @denesb which issue is it fixing?

bhalevy · 2023-05-29T13:55:22Z

@raphaelsc / @denesb which issue is it fixing?

@DoronArazii I don't think there is an issue.
I'll open one and immediately close it as fixed by the above.

mykaul · 2023-05-29T16:09:05Z

Is it considered a performance item, or should we consider backport?

mykaul · 2023-06-14T11:43:11Z

Is it considered a performance item, or should we consider backport?

@avikivity , @bhalevy ?

bhalevy · 2023-06-14T11:49:26Z

Is it considered a performance item, or should we consider backport?

It is performance related, as it can cause reactor stalls like in https://github.com/scylladb/scylla-enterprise/issues/2980#issuecomment-1590363663 (first stall)

Since it's low risk I'd vote for backporting it.
I'm not sure why it doesn't have a GH issue.

raphaelsc · 2023-06-14T12:23:49Z

Is it considered a performance item, or should we consider backport?

It is performance related, as it can cause reactor stalls like in scylladb/scylla-enterprise#2980 (comment) (first stall)

Since it's low risk I'd vote for backporting it. I'm not sure why it doesn't have a GH issue.

yes , it merits a backport. #14247

vladzcloudius · 2023-06-14T17:43:40Z

compaction/compaction.cc

-    os << "[";
-    os << boost::algorithm::join(lst._ssts, ",");
-    os << "]";
+    fmt::print(os, "[{}]", fmt::join(lst._ssts, ","));


Guys, why don't you simply print in the loop instead of joining this into a single huge string?

this is a lazy joiner.

Oh. I see. Ok.

vladzcloudius · 2023-06-14T17:44:44Z

compaction/compaction.cc

@@ -418,9 +418,12 @@ struct compaction_read_monitor_generator final : public read_monitor_generator {

 class formatted_sstables_list {
    bool _include_origin = true;
-    std::vector<sstring> _ssts;
+    std::vector<std::string> _ssts;


Why not to use std::list or std::deque or alike?
std::vector is a wrong container if you expect a frequent (more than 1) size change.

even our own chunked_vector is much better!

@denesb @raphaelsc

linked list is overkill, we know the size upfront, we don't have to reallocate. the container size rarely go beyond 1k, so the memory usage is bounded to ~100k bytes. we could have picked chunked_vector, but don't think it's a must.

1k -> 8kB

I was assuming ~100 bytes for each entry, as it duplicates the directory full path for each one of them.

we haven't seen any need to optimize this particular variable.
chunked_vector could indeed do a good job.
Feel free to send a patch for master (I doubt it'd be backported though)

Ah, and regarding vector vs. list; we do prefer a single allocation than yet another allocation per sstable.
Hence chunked_vector being a good compromise between a vector and a list.

we haven't seen any need to optimize this particular variable.

I see compactions of crazy amounts of sstables, especially with SAG and off-strategy.

So, in the light of #14241 1K items definitely doesn't sound like something impossible or even high enough to represent a ceiling. IMO we should have our code ready for hundreds of thousands if not millions. Will send a patch. NP.

it indeed doesn't hurt to switch to chunked_vector and it can handle extreme unexpected cases.

raphaelsc · 2023-07-02T15:46:05Z

@avikivity any objection on backporting this one?

…ting with off-strategy, input list size can be close to 1k, which will lead to unneeded reallocations when formatting the list for logging. in the past, we faced stalls in this area, and excessive reallocation (log2 ~1k = ~10) may have contributed to that. Signed-off-by: Raphael S. Carvalho <raphaelsc@scylladb.com> Closes #13907 (cherry picked from commit 5544d12) Fixes #14071

raphaelsc requested a review from nyh as a code owner May 16, 2023 12:57

raphaelsc requested a review from bhalevy May 16, 2023 12:57

bhalevy reviewed May 16, 2023

View reviewed changes

compaction/compaction.cc Show resolved Hide resolved

raphaelsc force-pushed the avoid-excessive-reallocation-when-formatting-input-compaction-list branch from ba72e94 to 462a9e0 Compare May 16, 2023 14:44

raphaelsc requested a review from bhalevy May 16, 2023 14:46

bhalevy approved these changes May 16, 2023

View reviewed changes

raphaelsc force-pushed the avoid-excessive-reallocation-when-formatting-input-compaction-list branch from 462a9e0 to f27268f Compare May 16, 2023 17:32

bhalevy approved these changes May 16, 2023

View reviewed changes

denesb approved these changes May 17, 2023

View reviewed changes

scylladb-promoter closed this in 5544d12 May 17, 2023

bhalevy mentioned this pull request May 29, 2023

compaction: excessive reallocation during input list formatting #14071

Closed

mykaul added the Backport candidate label Jun 14, 2023

vladzcloudius reviewed Jun 14, 2023

View reviewed changes

DoronArazii removed the Backport candidate label Jul 13, 2023

mykaul mentioned this pull request Aug 7, 2023

compaction: formatted_sstables_list can potentially cause OOM #14264

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compaction: avoid excessive reallocation during input list formatting #13907

compaction: avoid excessive reallocation during input list formatting #13907

raphaelsc commented May 16, 2023

raphaelsc commented May 16, 2023

bhalevy left a comment

scylladb-promoter commented May 16, 2023

raphaelsc commented May 16, 2023

scylladb-promoter commented May 16, 2023

DoronArazii commented May 28, 2023

bhalevy commented May 29, 2023

mykaul commented May 29, 2023

mykaul commented Jun 14, 2023

bhalevy commented Jun 14, 2023

raphaelsc commented Jun 14, 2023

vladzcloudius Jun 14, 2023

raphaelsc Jun 14, 2023

vladzcloudius Jun 14, 2023

vladzcloudius Jun 14, 2023

vladzcloudius Jun 14, 2023 •

edited

raphaelsc Jun 14, 2023

avikivity Jun 14, 2023

raphaelsc Jun 14, 2023

bhalevy Jun 15, 2023

bhalevy Jun 15, 2023

vladzcloudius Jun 15, 2023

raphaelsc Jun 15, 2023

vladzcloudius Jun 15, 2023

raphaelsc commented Jul 2, 2023

compaction: avoid excessive reallocation during input list formatting #13907

compaction: avoid excessive reallocation during input list formatting #13907

Conversation

raphaelsc commented May 16, 2023

raphaelsc commented May 16, 2023

bhalevy left a comment

Choose a reason for hiding this comment

scylladb-promoter commented May 16, 2023

raphaelsc commented May 16, 2023

scylladb-promoter commented May 16, 2023

DoronArazii commented May 28, 2023

bhalevy commented May 29, 2023

mykaul commented May 29, 2023

mykaul commented Jun 14, 2023

bhalevy commented Jun 14, 2023

raphaelsc commented Jun 14, 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

vladzcloudius Jun 14, 2023 • edited

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 Jul 2, 2023

vladzcloudius Jun 14, 2023 •

edited