Alexey Serbin 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG@24 PS2, Line 24: Measured with 'kudu perf loadgen --use_upsert --num_rows_per_thread=5M' : on a single-tablet cluster, median of N=5 runs: It would be great to see unit-level performance improvement for this for RELEASE build, without involving perf-loadgen. I'm not sure whether the result below are free of various noise not related to the code in question. Running unit-level bench under system `perf` tool would have much less noise and could provide better insights on the underlying perf improvement. http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG@27 PS2, Line 27: sorted input batch What sort of build was it? Please double-check it's a RELEASE, not a DEBUG one. http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG@29 PS2, Line 29: --use_random=false nit: there is no need to customize this, it's false by defalt http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG@31 PS2, Line 31: -buffer_flush_watermark_pct=1.0 nit: why to customize this at all? http://gerrit.cloudera.org:8080/#/c/24615/2//COMMIT_MSG@32 PS2, Line 32: --keep_auto_table=false nit: this is false by default already, can drop this useless customization 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@1175 PS2, Line 1175: if (is_sorted && !keys_and_indexes.empty() && : key < keys_and_indexes.back().first) { : is_sorted = false; : } Might invoking std::is_sorted() to decide whether to call std::stable_sort() below be a bit faster than evaluating these multiple conditions for every key? 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 optimisa +1: I don't see a reason to keep this as a metric for any reason beyond measuring performance just once. What value does this metric provide in a production cluster? 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_l For what purpose would anybody use these new metrics in production? -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Thu, 06 Aug 2026 06:34:22 +0000 Gerrit-HasComments: Yes
