Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24615 )
Change subject: [tablet] Skip stable_sort for already sorted batch ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1154 PS2, Line 1154: const MonoTime bulk_check_start = MonoTime::Now(); nit: bulk_check_start is computed unconditionally, but it's only consumed under `if (metrics_)` at line 1319. Worth guarding the MonoTime::Now() behind metrics_ (or accepting it as negligible) - trivial either way, but it's dead work when metrics_ is null. http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1179 PS2, Line 1179: keys_and_indexes.emplace_back(key, i); No test accompanies the change. Given the correctness argument rests on "non-decreasing input == valid stable-sort output," a small regression test asserting that a pre-sorted batch and its shuffled permutation yield identical presence results (and that the pre_sorted/needed_sort counters move as expected) would lock this in cheaply. Do existing bulk-check tests already cover both orderings? http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1203 PS2, Line 1203: sort_us = (MonoTime::Now() - sort_start).ToMicroseconds(); The commit message says these metrics exist "so that the sort-skip optimisation can be verified in isolation." Are all five intended to ship permanently, or are bulk_check_duration / bulk_check_sort_duration diagnostic scaffolding? This is a perf patch, yet it permanently adds 2-3 MonoTime::Now() calls plus several histogram/counter Increments to every write batch on the path it's optimizing. The per-op work dominates so the net benchmark is still positive, but it's worth being explicit about which of these we want long-term vs. what was just for validating the change. http://gerrit.cloudera.org:8080/#/c/24615/2/src/kudu/tablet/tablet.cc@1211 PS2, Line 1211: if (is_sorted) { q: a batch where every op was filtered out (has_result / orig_result_from_log) reaches here with keys_and_indexes empty, is_sorted still true, and gets counted as a "pre-sorted batch" with input_size 0. Minor, but it slightly inflates the pre-sorted counter with empty batches. Intended? -- To view, visit http://gerrit.cloudera.org:8080/24615 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I28d3f5ebdf7e3f13f04ffe8d35417a98c370fcf7 Gerrit-Change-Number: 24615 Gerrit-PatchSet: 2 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Sun, 02 Aug 2026 10:19:11 +0000 Gerrit-HasComments: Yes
