This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch branch-4.2
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-4.2 by this push:
new 9402fe03f5e branch-4.2: [improvement](be) Account the NULL docids and
point buffer of the SNII BKD writer #69728 (#69794)
9402fe03f5e is described below
commit 9402fe03f5e231c70d4d8136668736cad753d477
Author: Jack <[email protected]>
AuthorDate: Thu Oct 29 16:33:49 2026 +0800
branch-4.2: [improvement](be) Account the NULL docids and point buffer of
the SNII BKD writer #69728 (#69794)
### What problem does this PR solve?
Issue Number: None
Related PR: #69728
Problem Summary: Backport #69728 to `branch-4.2`.
`SniiBkdIndexColumnWriter`, the numeric, date and IP index writer,
installed no memory reporter. Its point buffer, and its NULL docid
vector, which grows with the row count, were invisible to the
process-wide SNII build tracker (the `SniiIndexBuild` MemTracker and the
`snii_index_build_mem_consumption` gauge) that the text writer feeds.
The writer now creates a reporter in the unregistered population, since
a forced spill cannot reclaim a BKD build, and hands it to the
BkdBuilder. It charges the NULL docids through
`reserve_docids_for_append`, a growth routine added next to
`MemoryReporter`: the larger capacity is charged before it is allocated,
and the old charge is returned after the move. It gives the vector and
its charge back as soon as `finish()` has moved the docids into the
bitmap, because IndexBuilder keeps finished writers alive until the
whole rowset is closed.
This only adds accounting. The reporter has no cap, so it cannot fail a
build, and its callback charges a plain MemTracker, not a limiter.
#### Branch adaptations
`branch-4.2` has the same BKD writer and the same `MemoryReporter`, so
the writer, header and test changes carry over. The `init()` hunk of
`snii_bkd_index_writer.cpp` needed a manual merge because `_add_value`
takes a `const void*` on this branch, and in the new test `add_values`
takes the column name as its first argument.
The text writer is not part of this backport. On `branch-4.2` it
accounts its NULL docids with `_report_null_docids_capacity()`, not with
the `Reservation`-based routine that master's text writer had when
#69728 extracted `reserve_docids_for_append`. So here the helper is
added next to `MemoryReporter` and only the BKD writer calls it, and
`snii_index_writer.cpp` is unchanged. The commit is based on master
`24dc7762f1`.
### Release note
The SNII index build memory metric now also counts the point buffers and
NULL rows of numeric, date and IP indexes.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- The test of #69728:
SniiBkdAdapterTest.BuildMemoryIsChargedAndGivenBackAtFinish.
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason
- Behavior changed:
- [x] No. The bytes are only reported to the build tracker, and are
returned at finish.
- [ ] Yes.
- Does this need documentation?
- [x] No.
- [ ] Yes.
### Validation
- `memory_reporter.h` and `snii_bkd_index_writer.h` take master's change
unchanged, `snii_bkd_index_writer.cpp` and the test were merged as
described above, and the text-writer hunk of `snii_index_writer.cpp` is
dropped.
- clang-format 16 on the four changed files.
- `snii_bkd_index_writer.cpp` and `snii_bkd_adapter_test.cpp` compile
(`-fsyntax-only`, with the flags of the BE unit-test build) against the
`branch-4.2` tree with these changes applied.
- CI on `branch-4.2` (`run buildall`) passed: compile, BE unit tests
(14044 passed, 107 ignored, including
`SniiBkdAdapterTest.BuildMemoryIsChargedAndGivenBackAtFinish`), P0,
cloud P0, vault P0, external and non-concurrent regression, and both
coverage checks (91.43% of the 35 changed lines).
- No local build of `branch-4.2` was run for this backport.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Sonnet 5.5 <[email protected]>
---
.../index/inverted/snii/snii_bkd_index_writer.cpp | 24 +++++++++++++
.../index/inverted/snii/snii_bkd_index_writer.h | 11 ++++++
.../index/inverted/snii/writer/memory_reporter.h | 35 ++++++++++++++++++
.../index/inverted/snii/snii_bkd_adapter_test.cpp | 41 ++++++++++++++++++++++
4 files changed, 111 insertions(+)
diff --git a/be/src/storage/index/inverted/snii/snii_bkd_index_writer.cpp
b/be/src/storage/index/inverted/snii/snii_bkd_index_writer.cpp
index c50b33181bb..677290fc2cd 100644
--- a/be/src/storage/index/inverted/snii/snii_bkd_index_writer.cpp
+++ b/be/src/storage/index/inverted/snii/snii_bkd_index_writer.cpp
@@ -17,6 +17,7 @@
#include "storage/index/inverted/snii/snii_bkd_index_writer.h"
+#include <algorithm>
#include <cstring>
#include <utility>
@@ -25,6 +26,7 @@
#include "storage/index/inverted/snii/bkd/bkd_types.h"
#include "storage/index/inverted/snii/format/metadata_directory.h"
#include "storage/index/inverted/snii/format/null_bitmap.h"
+#include "storage/index/inverted/snii/writer/snii_build_memory_tracker.h"
#include "storage/index/inverted/snii/writer/snii_compound_writer.h"
#include "storage/key_coder.h"
#include "storage/tablet/tablet_schema.h"
@@ -84,12 +86,26 @@ Status SniiBkdIndexColumnWriter::init() {
}
_value_size = cast_set<uint32_t>(field_type_size(_value_type));
+ // The same accounting the text writer has: the builder's point buffer and
the NULL docids are
+ // mirrored into the process-wide SNII build tracker. Unregistered,
because a forced spill cannot
+ // reclaim them -- the builder spills on its own buffer bound only.
+ _memory_reporter = std::make_unique<::doris::snii::writer::MemoryReporter>(
+ ::doris::snii::writer::snii_build_consume_release(
+
::doris::snii::writer::BuildMemoryPopulation::kUnregistered));
+ _null_docids_reservation = _memory_reporter->make_reservation();
+
::doris::snii::bkd::BkdBuilderOptions options;
options.bytes_per_dim = _value_size;
options.field_type = _value_type;
+ options.reporter = _memory_reporter.get();
return ::doris::snii::bkd::BkdBuilder::create(options, &_builder);
}
+Status SniiBkdIndexColumnWriter::_reserve_null_docids_for_append(size_t count)
{
+ return ::doris::snii::writer::reserve_docids_for_append(
+ _memory_reporter.get(), &_null_docids_reservation, &_null_docids,
count);
+}
+
Status SniiBkdIndexColumnWriter::_add_value(const void* value, uint32_t docid)
{
DORIS_CHECK(_builder != nullptr);
std::string encoded;
@@ -154,6 +170,7 @@ Status SniiBkdIndexColumnWriter::add_array_values(size_t
field_size, const void*
}
Status SniiBkdIndexColumnWriter::add_nulls(uint32_t count) {
+ RETURN_IF_ERROR(_reserve_null_docids_for_append(count));
for (uint32_t i = 0; i < count; ++i) {
_null_docids.push_back(_rid);
++_rid;
@@ -168,6 +185,9 @@ Status SniiBkdIndexColumnWriter::add_array_nulls(const
uint8_t* null_map, size_t
// array level.
DORIS_CHECK_GE(_rid, num_rows);
const uint32_t first_rid = _rid - cast_set<uint32_t>(num_rows);
+ const auto null_count =
+ static_cast<size_t>(std::count(null_map, null_map + num_rows,
static_cast<uint8_t>(1)));
+ RETURN_IF_ERROR(_reserve_null_docids_for_append(null_count));
for (size_t i = 0; i < num_rows; ++i) {
if (null_map[i] == 1) {
_null_docids.push_back(first_rid + cast_set<uint32_t>(i));
@@ -192,6 +212,10 @@ Status SniiBkdIndexColumnWriter::finish() {
::doris::snii::format::NullBitmapWriter null_writer;
null_writer.add_many(_null_docids);
+ // The docids live in the bitmap now. This writer can be kept alive long
after finish() returns,
+ // so give the vector and its charge back instead of holding them until
destruction.
+ std::vector<uint32_t>().swap(_null_docids);
+ _null_docids_reservation.reset();
::doris::snii::ByteSink null_sink;
RETURN_IF_ERROR(null_writer.finish(_rid, &null_sink));
diff --git a/be/src/storage/index/inverted/snii/snii_bkd_index_writer.h
b/be/src/storage/index/inverted/snii/snii_bkd_index_writer.h
index 972e42090fd..f914cdc8b1c 100644
--- a/be/src/storage/index/inverted/snii/snii_bkd_index_writer.h
+++ b/be/src/storage/index/inverted/snii/snii_bkd_index_writer.h
@@ -27,6 +27,7 @@
#include "storage/index/inverted/snii/bkd/bkd_builder.h"
#include "storage/index/inverted/snii/bkd/staged_blob_file.h"
#include "storage/index/inverted/snii/encoding/byte_sink.h"
+#include "storage/index/inverted/snii/writer/memory_reporter.h"
#include "storage/olap_common.h"
namespace doris {
@@ -84,9 +85,19 @@ private:
uint32_t _value_size = 0;
const KeyCoder* _value_key_coder = nullptr;
+ // Reserves room for `count` more NULL docids, charging the larger
capacity to the reporter first.
+ Status _reserve_null_docids_for_append(size_t count);
+
+ // Build-time RAM accounting, shared with the builder: the NULL docids
below and the builder's
+ // point buffer show up in the process-wide SNII build tracker while they
are held. Declared
+ // before everything that holds a Reservation on it, so it is destroyed
last.
+ std::unique_ptr<::doris::snii::writer::MemoryReporter> _memory_reporter;
+
// Segment-local row id of the NEXT row to arrive. Advanced by both value
// runs and null runs, so it is the row count once the column is exhausted.
uint32_t _rid = 0;
+ // Declared before the vector it charges for, so the vector is freed
before its charge is returned.
+ ::doris::snii::writer::MemoryReporter::Reservation
_null_docids_reservation;
std::vector<uint32_t> _null_docids;
std::unique_ptr<::doris::snii::bkd::BkdBuilder> _builder;
diff --git a/be/src/storage/index/inverted/snii/writer/memory_reporter.h
b/be/src/storage/index/inverted/snii/writer/memory_reporter.h
index 0f81821927c..68bc43385df 100644
--- a/be/src/storage/index/inverted/snii/writer/memory_reporter.h
+++ b/be/src/storage/index/inverted/snii/writer/memory_reporter.h
@@ -23,8 +23,10 @@
#include <functional>
#include <limits>
#include <utility>
+#include <vector>
#include "common/check.h"
+#include "common/compiler_util.h"
#include "common/logging.h"
#include "common/status.h"
@@ -327,4 +329,37 @@ inline void MemoryReporter::release(uint64_t bytes, bool
postings) {
}
}
+// Grows the vector for reserve_docids_for_append. Out of line: growth is
geometric, so it runs
+// O(log n) times, and keeping it out of the callers leaves them only the
capacity check.
+NO_INLINE inline Status grow_docids_for_append(MemoryReporter* reporter,
+ MemoryReporter::Reservation*
reservation,
+ std::vector<uint32_t>* docids,
size_t need) {
+ const size_t target_capacity = std::max(need, docids->capacity() * 2);
+ if (reporter == nullptr) {
+ docids->reserve(target_capacity);
+ return Status::OK();
+ }
+ MemoryReporter::Reservation replacement;
+ RETURN_IF_ERROR(reservation->prepare_replacement(
+ static_cast<uint64_t>(target_capacity) * sizeof(uint32_t),
&replacement));
+ docids->reserve(target_capacity);
+ *reservation = std::move(replacement);
+ return Status::OK();
+}
+
+// Makes room for `count` more docids in a vector whose capacity is charged to
`reservation`; a null
+// `reporter` (unit tests) charges nothing. Capacity grows geometrically, so a
long sequence of small
+// NULL runs does not reallocate and copy the whole array on every call. The
larger capacity is
+// charged BEFORE it is allocated and the old charge is returned only after
the move, which is the
+// transient double residency of a vector growth.
+inline Status reserve_docids_for_append(MemoryReporter* reporter,
+ MemoryReporter::Reservation*
reservation,
+ std::vector<uint32_t>* docids, size_t
count) {
+ const size_t need = docids->size() + count;
+ if (need <= docids->capacity()) {
+ return Status::OK();
+ }
+ return grow_docids_for_append(reporter, reservation, docids, need);
+}
+
} // namespace doris::snii::writer
diff --git a/be/test/storage/index/inverted/snii/snii_bkd_adapter_test.cpp
b/be/test/storage/index/inverted/snii/snii_bkd_adapter_test.cpp
index 4fcf89c18a0..7ae519c123d 100644
--- a/be/test/storage/index/inverted/snii/snii_bkd_adapter_test.cpp
+++ b/be/test/storage/index/inverted/snii/snii_bkd_adapter_test.cpp
@@ -21,6 +21,7 @@
#include <cstdint>
#include <memory>
+#include <numeric>
#include <optional>
#include <set>
#include <string>
@@ -29,6 +30,7 @@
#include "common/status.h"
#include "io/fs/local_file_system.h"
#include "runtime/exec_env.h"
+#include "runtime/memory/mem_tracker.h"
#include "runtime/runtime_state.h"
#include "storage/index/index_file_reader.h"
#include "storage/index/index_file_writer.h"
@@ -46,6 +48,7 @@
#include "storage/index/inverted/snii/snii_bkd_index_reader.h"
#include "storage/index/inverted/snii/snii_bkd_index_writer.h"
#include "storage/index/inverted/snii/staged_file_probe.h"
+#include "storage/index/inverted/snii/writer/snii_build_memory_tracker.h"
#include "storage/key_coder.h"
#include "storage/tablet/tablet_schema.h"
@@ -317,6 +320,44 @@ TEST_F(SniiBkdAdapterTest,
SealingDrainsTheStagedDataWhileTheProducerIsStillAliv
assert_ok(index_file_writer.finish_close());
}
+// The writer's build memory is visible in the process-wide SNII build tracker
while it is held, as
+// the text writer's is: the NULL docids until finish() moves them into the
bitmap, the builder's
+// point buffer until the build is done. None of it may stay charged once
finish() returns, even
+// though the producer is still alive (IndexBuilder keeps it until the rowset
is closed).
+TEST_F(SniiBkdAdapterTest, BuildMemoryIsChargedAndGivenBackAtFinish) {
+ ::doris::MemTracker* tracker =
::doris::snii::writer::snii_build_mem_tracker();
+ const int64_t baseline = tracker->consumption();
+ const int64_t registered_baseline =
::doris::snii::writer::snii_registered_build_bytes();
+
+ const std::string prefix = test_path("accounted_producer");
+ io::FileWriterPtr file_writer;
+ assert_ok(io::global_local_filesystem()->create_file(
+ InvertedIndexDescriptor::get_index_file_path_v2(prefix),
&file_writer));
+ IndexFileWriter index_file_writer(io::global_local_filesystem(), prefix,
"accounted_rowset",
+ /*seg_id=*/0,
InvertedIndexStorageFormatPB::SNII,
+ std::move(file_writer),
/*can_use_ram_dir=*/true,
+ /*tablet_id=*/303);
+ SniiBkdIndexColumnWriter writer(&index_file_writer, &_meta, kFieldType);
+ assert_ok(writer.init());
+
+ constexpr uint32_t kNulls = 200000;
+ assert_ok(writer.add_nulls(kNulls));
+ std::vector<int64_t> values(1000);
+ std::iota(values.begin(), values.end(), 0);
+ assert_ok(writer.add_values("c1", values.data(), values.size()));
+ // At least the 4 bytes of every NULL docid, and the builder's point
buffer on top.
+ EXPECT_GE(tracker->consumption() - baseline, static_cast<int64_t>(kNulls)
* 4);
+ // A BKD build cannot be reclaimed by a forced spill, so it is not offered
to the spill decision.
+ EXPECT_EQ(::doris::snii::writer::snii_registered_build_bytes(),
registered_baseline);
+
+ assert_ok(writer.finish());
+ EXPECT_EQ(tracker->consumption(), baseline)
+ << "finish() left build memory charged to a producer that is still
alive";
+
+ assert_ok(index_file_writer.begin_close());
+ assert_ok(index_file_writer.finish_close());
+}
+
// NULL rows are carried by the SNII null-bitmap POD, not by the point set: a
// NULL that leaked in as a point would answer `col > x` for a row that has no
// value at all.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
