50% system perf regression in memtable reader

[avikivity]

perf_simple_query --smp 1 shows a 50% performance drop after d8832b9. Maybe it's copying schema objects or something.

$ git bisect log
# bad: [4bd7019d0fd6140db05a45d069a7df71b208adb8] Update seastar submodule
# good: [0ea79559a669259405265194aa33c1b8f7456ecc] Merge 'IDL: support generating boilerplate code for RPC verbs' from Pavel Solodovnikov
git bisect start 'next' 'HEAD' '--first-parent'
# good: [ee8dc6847c051e6b7e561710b1c78cf19f299c3a] scylla.yaml: refresh list of experimental features
git bisect good ee8dc6847c051e6b7e561710b1c78cf19f299c3a
# bad: [aa4aba40aa0ee788a435c5e5a9f333c610700a96] sstables: sstable_run: introduce estimate_droppable_tombstone_ratio
git bisect bad aa4aba40aa0ee788a435c5e5a9f333c610700a96
# bad: [acfe0a3803f576086f27b2abc7fe7d16a91d93f7] build: reinstate -Wunknown-attributes
git bisect bad acfe0a3803f576086f27b2abc7fe7d16a91d93f7
# bad: [4f3b8f38e24b1ca78c356dd4fb70365bb3cdfded] Merge "Add effective_replication_map" from Benny
git bisect bad 4f3b8f38e24b1ca78c356dd4fb70365bb3cdfded
# bad: [d8832b9fd8767e9831ceefa4205ac892fa496fa9] Merge 'Memtable make reversing reader' from Michał Radwański
git bisect bad d8832b9fd8767e9831ceefa4205ac892fa496fa9
# first bad commit: [d8832b9fd8767e9831ceefa4205ac892fa496fa9] Merge 'Memtable make reversing reader' from Michał Radwański

[enedil]

I identified the issue. This patch fixes it:

$ git diff
diff --git a/partition_snapshot_reader.hh b/partition_snapshot_reader.hh
index 65d0f3670..85c7ff0c6 100644
--- a/partition_snapshot_reader.hh
+++ b/partition_snapshot_reader.hh
@@ -114,7 +114,7 @@ class partition_snapshot_flat_reader : public flat_mutation_reader::impl, public
         // In reversing mode, upper and lower bounds still need to be executed against
         // snapshot schema and ck_range, however we need them to search from "opposite" direction.
         template<typename T, typename... Args>
-        static rows_iter_type lower_bound(const T& t, Args... args) {
+        static rows_iter_type lower_bound(const T& t, Args&&... args) {
             if constexpr (Reversing) {
                 return make_iterator(t.upper_bound(std::forward<Args>(args)...));
             } else {
@@ -122,7 +122,7 @@ class partition_snapshot_flat_reader : public flat_mutation_reader::impl, public
             }
         }
         template<typename T, typename... Args>
-        static rows_iter_type upper_bound(const T& t, Args... args) {
+        static rows_iter_type upper_bound(const T& t, Args&&... args) {
             if constexpr (Reversing) {
                 return make_iterator(t.lower_bound(std::forward<Args>(args)...));
             } else {

I will submit it in a moment.

The issue was, that one of the parameters was of type const schema&, which got expanded as schema to parameter list, causing schema copy constructor to execute.

[avikivity]

Perhaps we should make schema::schema(const schema&) private and expose it as schema::clone(). This ensures that there are no accidental copies.

[avikivity]

No vulnerable branches, not backporting.