From c4e71fb9b80aacc3ca53841a4e945137d15779df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Botond=20D=C3=A9nes?= Date: Fri, 11 Jun 2021 14:20:20 +0300 Subject: [PATCH] reader_concurrency_semaphore: remove default name parameter Naming the concurrency semaphore is currently optional, unnamed semaphores defaulting to "Unnamed semaphore". Although the most important semaphores are named, many still aren't, which makes for a poor debugging experience when one of these times out. To prevent this, remove the name parameter defaults from those constructors that have it and require a unique name to be passed in. Also update all sites creating a semaphore and make sure they use a unique name. --- database.cc | 2 +- reader_concurrency_semaphore.hh | 6 +++++- sstables/sstables.cc | 4 ++-- test/lib/reader_lifecycle_policy.hh | 7 +++++-- test/lib/reader_permit.cc | 2 +- 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/database.cc b/database.cc index c8d1954224..b7a4904dde 100644 --- a/database.cc +++ b/database.cc @@ -338,7 +338,7 @@ database::database(const db::config& cfg, database_config dbcfg, service::migrat max_memory_streaming_concurrent_reads(), "_streaming_concurrency_sem") // No limits, just for accounting. - , _compaction_concurrency_sem(reader_concurrency_semaphore::no_limits{}) + , _compaction_concurrency_sem(reader_concurrency_semaphore::no_limits{}, "compaction") , _system_read_concurrency_sem( // Using higher initial concurrency, see revert_initial_system_read_concurrency_boost(). max_count_concurrent_reads, diff --git a/reader_concurrency_semaphore.hh b/reader_concurrency_semaphore.hh index 8363bede86..b29681134e 100644 --- a/reader_concurrency_semaphore.hh +++ b/reader_concurrency_semaphore.hh @@ -198,6 +198,9 @@ private: public: struct no_limits { }; + /// Create a semaphore with the specified limits + /// + /// The semaphore's name has to be unique! reader_concurrency_semaphore(int count, ssize_t memory, sstring name, @@ -207,7 +210,8 @@ public: /// Create a semaphore with practically unlimited count and memory. /// /// And conversely, no queue limit either. - explicit reader_concurrency_semaphore(no_limits, sstring name = "unlimited reader_concurrency_semaphore"); + /// The semaphore's name has to be unique! + explicit reader_concurrency_semaphore(no_limits, sstring name); ~reader_concurrency_semaphore(); diff --git a/sstables/sstables.cc b/sstables/sstables.cc index 9c0eb90d1b..bd7f274d61 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -1774,7 +1774,7 @@ future<> sstable::generate_summary(const io_priority_class& pc) { }; return new_sstable_component_file(_read_error_handler, component_type::Index, open_flags::ro).then([this, &pc] (file index_file) { - return do_with(std::move(index_file), std::make_unique(reader_concurrency_semaphore::no_limits{}), + return do_with(std::move(index_file), std::make_unique(reader_concurrency_semaphore::no_limits{}, "sstables::generate_summary()"), [this, &pc] (file index_file, std::unique_ptr& sem) { return index_file.size().then([this, &pc, index_file, &sem] (auto index_size) { // an upper bound. Surely to be less than this. @@ -2679,7 +2679,7 @@ future sstable::has_partition_key(const utils::hashed_key& hk, const dht:: if (!filter_has_key(hk)) { return make_ready_future(false); } - auto sem = std::make_unique(reader_concurrency_semaphore::no_limits{}); + auto sem = std::make_unique(reader_concurrency_semaphore::no_limits{}, "sstables::has_partition_key()"); auto lh_index_ptr = std::make_unique(s, sem->make_permit(_schema.get(), s->get_filename()), default_priority_class(), tracing::trace_state_ptr()); auto& lh_index = *lh_index_ptr; return lh_index.advance_lower_and_check_if_present(dk).then([lh_index_ptr = std::move(lh_index_ptr), s, sem = std::move(sem)] (bool present) mutable { diff --git a/test/lib/reader_lifecycle_policy.hh b/test/lib/reader_lifecycle_policy.hh index 37daf31b16..41e3d3ac7c 100644 --- a/test/lib/reader_lifecycle_policy.hh +++ b/test/lib/reader_lifecycle_policy.hh @@ -90,11 +90,14 @@ public: } else if (_contexts[shard]->semaphore) { return *_contexts[shard]->semaphore; } + // To support multiple reader life-cycle instances alive at the same + // time, incorporate `this` into the name, to make their names unique. + auto name = format("tests::reader_lifecycle_policy@{}@shard_id={}", fmt::ptr(this), shard); if (_evict_paused_readers) { // Create with no memory, so all inactive reads are immediately evicted. - _contexts[shard]->semaphore.emplace(1, 0, format("reader_concurrency_semaphore @shard_id={}", shard)); + _contexts[shard]->semaphore.emplace(1, 0, std::move(name)); } else { - _contexts[shard]->semaphore.emplace(reader_concurrency_semaphore::no_limits{}); + _contexts[shard]->semaphore.emplace(reader_concurrency_semaphore::no_limits{}, std::move(name)); } return *_contexts[shard]->semaphore; } diff --git a/test/lib/reader_permit.cc b/test/lib/reader_permit.cc index a58e9a9ec8..d77825b646 100644 --- a/test/lib/reader_permit.cc +++ b/test/lib/reader_permit.cc @@ -24,7 +24,7 @@ namespace tests { -thread_local reader_concurrency_semaphore the_semaphore{reader_concurrency_semaphore::no_limits{}}; +thread_local reader_concurrency_semaphore the_semaphore{reader_concurrency_semaphore::no_limits{}, "global_test_semaphore"}; reader_concurrency_semaphore& semaphore() { return the_semaphore;