Querier: optimize moves of querier_opt, partition_range and partition_slice #18633

bhalevy · 2024-05-12T19:43:24Z

This series contains a couple of optimizations for the querier interface
that optimize optional<querier> moves and packs the querier dht::partition_range and query::partition_alice
into a new querier_reader_context structure that uses a single allocation.

Total improvement as measured with perf-simple-query is of about 400 instructions/op (~1%) and 1 fewer allocs/op:

base: 94575.94 tps ( 63.1 allocs/op,   0.0 logallocs/op,  14.2 tasks/op,   42288 insns/op,        0 errors)
post: 96538.66 tps ( 62.1 allocs/op,   0.0 logallocs/op,  14.1 tasks/op,   41895 insns/op,        0 errors)

No backport required, this PR contains only optimizations

bhalevy · 2024-05-12T19:45:05Z

perf results obtained with #18628

avikivity · 2024-05-12T20:16:44Z

replica/database.cc

- querier_opt = _querier_cache.lookup_data_querier(cmd.query_uuid, *s, ranges.front(), cmd.slice, semaphore, trace_state, timeout);
+ if (auto found = _querier_cache.lookup_data_querier(cmd.query_uuid, *s, ranges.front(), cmd.slice, semaphore, trace_state, timeout)) {
+ querier_opt.emplace(std::move(*found));
+ }


How does this improve anything? Usually lookup_data_querier returns nullopt and operator= will do almost nothing. Moreover, the optimizer should remember that querier_opt is disengaged.

I agree, this just makes the code slightly harder to read, without any apparent gain.

true, it has very little effect.
I needed it at some point in development when the copy constructor was deleted.
But it's not a must to have.

avikivity · 2024-05-12T20:19:19Z

replica/table.cc

 }

 while (!qs.done()) {
 auto&& range = *qs.current_partition_range++;

 if (!querier_opt) {
 query::querier_base::querier_config conf(_config.tombstone_warn_threshold);
- querier_opt = query::querier(as_mutation_source(), s, permit, range, qs.cmd.slice, trace_state, conf);
+ querier_opt.emplace(as_mutation_source(), s, permit, range, qs.cmd.slice, trace_state, conf);


This I understand.

avikivity · 2024-05-12T20:21:01Z

replica/table.cc

- if (saved_querier) {
- *saved_querier = std::move(querier_opt);
+ if (saved_querier && querier_opt) {
+ saved_querier->emplace(std::move(*querier_opt));
 }


Alternative approach: make querier a unique_ptr and (and use optimized_optional, or just unique_ptr's optionalness). De-pointerize the contents of queriers.

I'm not sure it's that simple. Maybe with same massage to the multishard querier.
The root problem is the separation of the readers that may be kept as inactive reads by the reader concurrency semaphore, and state they rely on, like the partition range and slice. Those are kept external to the reader using mechanisms like lifecycle policy. Some readers do keep some of the state (see the evictable_reader_v2) and in the case of the shard_mutation_querier, the range and slice are kept long term in reader_meta::remote_parts so they can be used for recreating the reader in case it was evicted.
Maybe the querier impl can be kept as a whole in remote_parts, but that might conflict with the querier cache.
I wish we had one mechanism for saving the query state that would work for all use cases.

Ah, and we also have query_state in table::query that keep partition ranges, but it is not kept long term.

avikivity · 2024-05-12T20:23:35Z

querier.cc

@@ -121,7 +121,7 @@ static bool ranges_match(const schema& s, const dht::partition_range& original_r
 return bound_eq(original_range.end(), new_range.end());
 }

-static bool ranges_match(const schema& s, dht::partition_ranges_view original_ranges, dht::partition_ranges_view new_ranges) {
+static bool ranges_match(const schema& s, const dht::partition_ranges_view& original_ranges, const dht::partition_ranges_view& new_ranges) {


partition_ranges_view is just two words (btw - can be replaced with std::span now). Passing it is cheap while dereferencing it is more expensive. I don't think this is an optimization (unless - the compiler refuses to pass it in registers).

I'll measure perf-simple-query on this change alone.

Hmm, it would be up to 30 instruction/op give or take a few as the next patch shows...
so I'll drop both

avikivity · 2024-05-12T20:26:18Z

querier.cc

+ }
+
+ return std::nullopt;
+}


I don't believe this complication is worth 30 instructions.

makes sense

avikivity · 2024-05-12T20:29:51Z

querier.hh

@@ -65,29 +72,24 @@ public:
 protected:
 schema_ptr _schema;
 reader_permit _permit;
- lw_shared_ptr<const dht::partition_range> _range;
- std::unique_ptr<const query::partition_slice> _slice;
+ querier_reader_context_ptr _ctx;
 std::variant<flat_mutation_reader_v2, reader_concurrency_semaphore::inactive_read_handle> _reader;


We could move all of the querier into an allocated object. Then moving queriers around is just moving unique_ptrs.

We could be even crazier and move all the contents into reader_permit (and then querier becomes a small wrapper around it). But we'll have many more reader_permit:s than queriers.

scylladb-promoter · 2024-05-12T23:11:37Z

🔴 CI State: FAILURE

✅ - Build
✅ - Container Test
✅ - dtest with topology changes
✅ - dtest
❌ - Unit Tests

Failed Tests (1/30735):

[empty] 🔍

Build Details:

Duration: 3 hr 26 min
Builder: i-08d76d2e3e5421cc2 (m6id.8xlarge)

denesb

Some of the changes are good, but others has gone too far, in particular patch 3 (and to some extent, parts of patch 1). The readability and maintainability of the code is also something we should also optimize for. On the hot path we are willing to sacrifice some of the readability and maintainability for performance but the tradeoff should still be reasonable.

denesb · 2024-05-13T04:30:02Z

replica/database.cc

- querier_opt = _querier_cache.lookup_data_querier(cmd.query_uuid, *s, ranges.front(), cmd.slice, semaphore, trace_state, timeout);
+ if (auto found = _querier_cache.lookup_data_querier(cmd.query_uuid, *s, ranges.front(), cmd.slice, semaphore, trace_state, timeout)) {
+ querier_opt.emplace(std::move(*found));
+ }


I agree, this just makes the code slightly harder to read, without any apparent gain.

replica/table.cc

denesb · 2024-05-13T04:52:36Z

querier.cc

+ }
+
+ return std::nullopt;
+}


Save a move of the querier object. Suprisingly, this yields a saving of about 100 instructions/op: ``` base: 94575.94 tps ( 63.1 allocs/op, 0.0 logallocs/op, 14.2 tasks/op, 42288 insns/op, 0 errors) post: 93323.42 tps ( 63.1 allocs/op, 0.0 logallocs/op, 14.2 tasks/op, 42181 insns/op, 0 errors) ``` Signed-off-by: Benny Halevy <[email protected]>

Keep both partition_range and partition_slice in a single allocated structure to reduce the number of allocations and clean up the code a bit in this area, since the mutlishard mutation query always keeps both in the reader_meta::remote_parts as a pair. This change saves an allocation call and it reduces copies of range and slice, yielding close to 300 insns/op improvement in perf-simple-query: ``` base: 93323.42 tps ( 63.1 allocs/op, 0.0 logallocs/op, 14.2 tasks/op, 42181 insns/op, 0 errors) post: 96538.66 tps ( 62.1 allocs/op, 0.0 logallocs/op, 14.1 tasks/op, 41895 insns/op, 0 errors) ``` Signed-off-by: Benny Halevy <[email protected]>

bhalevy · 2024-05-13T11:43:25Z

v2: simplified series

scylladb-promoter · 2024-05-13T17:58:39Z

🔴 CI State: FAILURE

✅ - Build
✅ - Container Test
❌ - dtest with topology changes
✅ - dtest
❌ - Unit Tests

Failed Tests (4/30696):

Build Details:

Duration: 6 hr 15 min
Builder: i-09815ad4dc3c21d12 (r5ad.8xlarge)

bhalevy added the backport/none Backport is not required label May 12, 2024

bhalevy requested review from denesb and tgrabiec as code owners May 12, 2024 19:43

bhalevy force-pushed the querier-optimize-partition_range_view branch from 60a8b91 to d2e2b09 Compare May 12, 2024 19:44

avikivity reviewed May 12, 2024

View reviewed changes

denesb reviewed May 13, 2024

View reviewed changes

bhalevy added 2 commits May 13, 2024 14:09

bhalevy force-pushed the querier-optimize-partition_range_view branch from d2e2b09 to 37839ab Compare May 13, 2024 11:42

bhalevy changed the title ~~[RFC] Querier: optimize for single range queries~~ Querier: optimize moves of querier_opt, partition_range and partition_slice May 13, 2024

bhalevy requested review from denesb and avikivity May 13, 2024 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Querier: optimize moves of querier_opt, partition_range and partition_slice #18633

Querier: optimize moves of querier_opt, partition_range and partition_slice #18633

bhalevy commented May 12, 2024 •

edited

bhalevy commented May 12, 2024

avikivity May 12, 2024

denesb May 13, 2024

bhalevy May 13, 2024

avikivity May 12, 2024

avikivity May 12, 2024

bhalevy May 13, 2024

avikivity May 12, 2024

bhalevy May 13, 2024

bhalevy May 13, 2024

avikivity May 12, 2024

denesb May 13, 2024

bhalevy May 13, 2024

avikivity May 12, 2024

scylladb-promoter commented May 12, 2024

denesb left a comment

denesb May 13, 2024

denesb May 13, 2024

bhalevy commented May 13, 2024

scylladb-promoter commented May 13, 2024

Querier: optimize moves of querier_opt, partition_range and partition_slice #18633

Are you sure you want to change the base?

Querier: optimize moves of querier_opt, partition_range and partition_slice #18633

Conversation

bhalevy commented May 12, 2024 • edited

bhalevy commented May 12, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scylladb-promoter commented May 12, 2024

🔴 CI State: FAILURE

Failed Tests (1/30735):

Build Details:

denesb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhalevy commented May 13, 2024

scylladb-promoter commented May 13, 2024

🔴 CI State: FAILURE

Failed Tests (4/30696):

Build Details:

bhalevy commented May 12, 2024 •

edited