[auto-perf-opt] iter-041 stack: LakeDelvecLoader fix on top of validated stack#72669
Closed
luohaha wants to merge 24 commits into
Closed
[auto-perf-opt] iter-041 stack: LakeDelvecLoader fix on top of validated stack#72669luohaha wants to merge 24 commits into
luohaha wants to merge 24 commits into
Conversation
… PK-index compaction CPU
Profile of cloud_native_pk_index_compact under realtime-benchmark
999_1gb_1_1tb workload (perf -F 99 -e cpu-clock 60s on the hottest
CN, 6 nodes, 32 cores each, all 6 saturated at 99-100% non-idle CPU)
shows my_free at 38.6% self-time with the dominant call paths inside
KeyValueMerger::merge -> KeyValueMerger::flush and inside
~IndexValuesWithVerPB destruction. Inclusive view:
78.91% LakePersistentIndexParallelCompactTask::run
55.26% KeyValueMerger::merge
26.59% KeyValueMerger::flush
9.63% my_free (dtor of local index_value_pb)
9.62% operator new (RepeatedField growth)
4.92% ~IndexValuesWithVerPB
1.76% sstable::TableBuilder::Add
11.62% my_free (dtor of local index_value_ver)
11.18% MessageLite::ParseFromString (10.72% in operator new)
5.45% ~IndexValuesWithVerPB
Both `merge()` and `flush()` allocate a fresh IndexValuesWithVerPB on
the stack per call and let it destruct at scope exit, freeing all of
the protobuf's internal RepeatedField storage even though the next
call needs an identically-shaped message. With one merger per
compact task and one merge() call per merged key, this is millions
of malloc/free pairs per task — at the per-task exec time of 46s
observed in iteration 001, allocator churn alone accounts for the
majority of compaction CPU.
Fix: hoist the two protobuf messages plus the SerializeAsString
output buffer to KeyValueMerger members, and Clear() / SerializeToString()
into them per call. Clear() preserves RepeatedField capacity, so
subsequent ParseFromString reuses already-grown internal arrays
instead of going back to the allocator. KeyValueMerger is constructed
once per LakePersistentIndexParallelCompactTask and is single-threaded
within the task, so no synchronization is needed.
Expected impact: based on the profile, removing the in-loop allocator
churn should shave 30-50% CPU off cloud_native_pk_index_compact tasks,
reducing per-task exec time and freeing CPU for the publish_version /
async_delta_writer / cloud_native_pk_index_execution pools that share
the same cores. Iteration 002 of auto-perf-opt my-rt-2 will deploy
and measure.
Related: supersedes StarRocks#72428, which capped the compact thread count at
num_cores. Throttling a CPU-bound pool that is doing real work makes
read-amplification worse over time; making each unit of work cheaper
is the correct fix.
Signed-off-by: auto-perf-opt <casey.luo@celerdata.com>
… loop Why: 999_1gb_1_1tb iter-005 on branch-4.1+PR StarRocks#72434 (= base + KeyValueMerger protobuf-scratch reuse) regressed from iter-003: upsert P99 1297 -> 1531 ms (+18 %), throughput 788 K -> 586 K rows/s (-26 %). A 60 s perf -F 99 -e cpu-clock profile on the hottest CN (172.26.80.125, 31.5 / 32 cores busy mid-test) shows allocator self-time at 24 % (my_malloc 12.14 % + my_free 11.91 %), with the dominant call paths now landing inside LakePersistentIndexParallelCompactTask::do_run and KeyValueMerger::merge through `Slice::to_string()`, not through the IndexValuesWithVerPB lifecycle that StarRocks#72434 already fixed. Inclusive view (cloud_native_pk thread family): 31.15 % start_thread 23.30 % LakePersistentIndexParallelCompactTask::run 19.90 % do_run 8.81 % KeyValueMerger::merge 4.70 % KeyValueMerger::flush 6.55 % operator new (do_run direct) -> cur_key.to_string() 3.12 % my_free (do_run direct) -> ~std::string Three call sites still convert iterator-owned slices to fresh std::strings on every input row, even though the iterator's slice storage is stable until the next ->Next() and the only consumers are ParseFrom* / Slice-based comparisons: KeyValueMerger::merge - key.to_string() + value.to_string() (line 39-40) parallel_compact_mgr::do_run - cur_key.to_string() (line 200) LakePersistentIndex::merge_sstables - cur_key.to_string() (line 641) What this changes: keep the iterator-owned Slice as-is. - merge() parses via ParseFromArray(value.data, value.size) (no value copy) and compares via Slice(_key) == key (the cached key stays a std::string only because the merger outlives the per-iteration slice). - The branch that actually changes _key still pays a single _key.assign(key.data, key.size) - once per distinct output key, not once per input row. - do_run / merge_sstables drop the cur_key string entirely; they only feed cur_key into the comparator, which already takes a Slice. Stacked on PR StarRocks#72434 (cherry-picked into this branch) so the cumulative deploy carries both wins. Risk: minimal. Slice values reference iterator-owned storage stable until iter_ptr->Next(); parse + comparison both finish before Next() is called. Slice(const std::string&) is the standard zero-copy adapter used throughout this codebase. Public API and on-disk format unchanged. Expected impact: removes the dominant remaining per-key heap allocation pair on the parallel PK-index compaction thread. Profile-implied savings ~10 % of total CPU on the hot CN (the do_run-direct alloc/free pair) plus a slice of the 8.81 % KeyValueMerger::merge inclusive cost. Should restore iter-003's 788 K rows/s throughput and pull upsert P99 back below 1.3 s.
The PK-index / update-state caches in `UpdateManager` are time-evicted after `update_cache_expire_sec` (default 360s = 6 min). This is short enough that any brief idle gap drains `_index_cache` of all tablets, and when traffic resumes, every tablet's first publish takes the full cold-load path simultaneously: `_open_sstables_parallel` opens 15-24 SSTs per tablet and pulls each ~2 MB filter block from local block cache. With ~1000 active tables = ~20K concurrent SST opens, local SSD saturates at 96-97% util and per-publish `primary_index_load_latency_us` balloons to 6-16 s. Measured on the my-rt-2 999_1gb_1_1tb workload (49 min idle gap between test windows, branch-4.1@ab2361c): - BE publish-trace cold-start max = 16.8 s on noisiest CN - 436 of 1016 SLOW_LOAD's `CommitAndPublishTimeMs > 5 s` - Upsert max 14.5 s vs goal of 3 s - Local disk vdb %util peaks 96.9% during the 30 s thunder-herd - After warm-up, P50 publish drops to 1-5 ms - steady-state is fine Bumping to 7200 s (2 h) keeps PK-index entries warm through normal diurnal patterns. Memory safety is unchanged: `UpdateManager::evict_cache` still enforces memory limits via `memory_urgent_level / memory_high_level` thresholds, so the longer time bound cannot cause OOM. For genuinely long idle windows (overnight, multi-hour deploys), entries still expire and free memory. Affects both shared-data (`lake::UpdateManager`) and shared-nothing (`UpdateManager`) since both `set_cache_expire_ms` calls in `olap_server.cpp` derive their value from this single config.
…SEPORT
Problem
-------
Each EvHttpServer worker thread runs its own libevent event_base, but they
all share a single listening socket (`_server_fd`). Because libevent does
not pass `EPOLLEXCLUSIVE`, every incoming SYN wakes *every* worker's
epoll_wait, and they all call `accept4()` simultaneously. Only one
succeeds; the rest get `EAGAIN` after serializing on the kernel listening
socket's `lock_sock_nested` / `_raw_spin_lock_bh`.
At `be_http_num_workers = 1000` this kernel spinlock contention dominates
HTTP-server CPU. perf on a busy CN during ingest:
8.57% http_server starrocks_be event_base_loop (incl)
7.69% http_server starrocks_be listener_read_cb (incl)
7.69% http_server libc.so accept4 (incl)
7.50% http_server [kernel] native_queued_spin_lock_slowpath <-- self
7.30% http_server [kernel] inet_csk_accept
7.24% http_server [kernel] lock_sock_nested
The HTTP-pickup gap this contention introduces shows up in stream-load
diagnostics as "outside-BE-dominant" slow loads — total latency is
dominated by the gap between request arrival and BE handling, not by
BE-side WriteData / CommitAndPublish.
Fix
---
Add `enable_http_server_so_reuseport` (default `true`). When enabled, each
worker `i > 0` creates its own listening socket with `SO_REUSEPORT` bound
to the same `{host, real_port}`, and the kernel load-balances new
connections across these sockets in-kernel — no more wake-all + race-on-
accept. Worker 0 keeps using the existing `_server_fd` from `_bind()`,
which has `SO_REUSEPORT` set so the additional listeners are accepted by
the kernel. On any setsockopt / bind failure (older kernels, EADDRINUSE)
the code falls back to the legacy shared-fd path so behaviour is
preserved end-to-end.
`stop()` and `join()` are extended to shutdown / close the per-worker
listeners alongside the existing primary fd.
Impact
------
This is the only `butil::tcp_listen` site in BE and only affects the HTTP
server's accept loop. Stream-load handler thread pools, brpc Server, and
the request lifecycle are untouched. With `be_http_num_workers >> 1` the
kernel-spinlock self-time on the HTTP-pickup hot path should drop to near
zero; worker-local accept queues replace the shared one.
iter-022 + iter-023 paired-measurement of FE-recorded "publish rpc cost" vs BE-recorded handler `cost=` shows a real ~1.2-3.0 s gap on the slow tail (max 3032 ms gap at FE 3210 ms vs BE 178 ms). 95 of 95 slow FE-rpc events are coordinated by BE 172.26.80.128, dispatched fan-out to all 6 destination BEs. The gap dominates the BE handler cost, so the time is NOT being spent inside the publish_version handler itself. Three remaining suspects for the gap: (1) BE-side BRPC server queueing before handler dispatch (2) BE-side response transmission after handler completes (3) FE / coordinator-BE BRPC client serialization or queueing This patch instruments suspect (1): record `cntl->latency_us()` at handler entry (which on the server side is the queue time before dispatch per brpc/controller.h:204-212), and append it to the existing slow-publish log line as `brpc_queue_us=...`. The line only fires when publish is already classified slow (>= lake_publish_version_slow_log_ms), so steady-state log volume is unchanged. iter-024 will deploy this and read brpc_queue_us on slow events to discriminate (1) vs (2)/(3).
Add a single WARN log line in PublishVersionDaemon.publishPartition that fires only when the total wall-clock from publish-task submission to BRPC return exceeds 500ms, breaking it into four sub-phases: - executor_acquire_ms : publish-task ThreadPoolExecutor queue wait - lock_wait_ms : DB intensive read-lock acquire (lockTablesWithIntensiveDbLock) - fe_prep_ms : table/partition/tablet lookup under the lock - brpc_ms : Utils.publishVersion(...) round-trip (BRPC + BE handler) These four phases sum to publishPartition's contribution to the existing "publish rpc cost" emitted by TransactionState.toString() in the VISIBLE log line. Steady-state publishes (< 500ms) log nothing — log volume on the hot path is unchanged. Motivation ---------- Across iter-022/023/024/025 we have observed a stable 1-3s gap between the FE-measured "publish rpc cost" and the maximum BE-measured handler cost on slow upsert events. iter-024 ruled out BRPC server-side queueing (brpc_queue_us ≤ 1ms on slow handlers) and the 1.14M-event survey showed the BE handler accounts for less than 0.1% of slow latency. The gap therefore lives on the FE side, but the existing measurement window is opaque — we cannot distinguish executor-pool queueing from lock contention from the per-partition BRPC fan-out. This patch is a 15-LOC, log-only instrumentation that surfaces the dominant phase the next time a slow event reproduces, so the actual fix can be targeted instead of guessed. Performance ----------- - Five System.currentTimeMillis() calls per partition publish (six µs each). - One LOG.warn invocation only on >= 500ms tails (~10-100 events / 20-min benchmark vs ~1M VISIBLE lines). - No change to the BRPC call site, no change to thread-pool config, no change to the existing "publish rpc cost" log field.
…atch
Add a single WARN log line in com.starrocks.lake.Utils.publishVersionBatch
that fires only when wall-clock from method entry to all-responses-received
exceeds 500ms, breaking it into three sub-phases:
- process_tablets_ms : processTablets(...) - groups tablets by ComputeNode
under various GlobalStateMgr / WarehouseManager
lookups; could lock-contend under burst.
- rpc_submit_ms : the per-node submit loop. For each destination,
BrpcProxy.getLakeService(...) + lakeService
.publishVersion(request). The submit is async so
this loop should be fast; it covers BRPC channel
lookup / lazy creation if any.
- rpc_wait_ms : the responseList.get(i).get() loop, i.e.
max(BE response time) including client-side
response decode.
Steady-state publishes (< 500ms) log nothing - log volume on the hot path is
unchanged.
Motivation
----------
Iter-025 (PR StarRocks#72557) added per-partition phase logging in PublishVersionDaemon
.publishPartition and proved that 100% of slow publishPartition wall time
lives in `brpc_ms` = `Utils.publishVersion(...)` - none in executor-acquire,
lock-wait, or fe_prep. Across a single 20-min benchmark this fired on 10,448
in-test events, p99=10.9s, max=25.3s, with 75% in the cold-start ramp-up
window (first 3 min after a fresh deploy).
iter-024 already proved BRPC server-side queueing is < 0.1% of slow latency,
and BE handler cost on slow events is 200-300ms while FE wall is 1.5-25s. The
gap is therefore inside `publishVersionBatch` itself but the existing
measurement window cannot tell us which of the three sub-phases dominates.
This patch is a 4-LOC, log-only instrumentation that surfaces the dominant
sub-phase the next time the storm reproduces, so the actual fix can be
targeted instead of guessed (per the program's "trace over guess" rule).
Performance
-----------
- Four System.currentTimeMillis() calls per publishVersionBatch invocation
(one extra branch on the threshold check).
- One LOG.warn invocation only on >= 500ms tails (~10K / 20-min cold-start;
near-zero in steady state vs ~M VISIBLE log lines).
- No change to the BRPC call site, no change to thread-pool config, no
change to existing log fields.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…:load_dels
Cold-start PK index load on shared-data clusters with `file_bundling=true`
spends most of its wall-clock either opening SSTs or replaying delete files
on the publish_version critical path. iter-027 captured one tablet's slow
publish at 20.16s with the following sub-times:
primary_index_load_latency_us: 18.47 s
├── pindex_init_us: 8.26 s (already parallel via
│ _open_sstables_parallel)
└── pindex_load_from_lake_tablet: 10.21 s
└── rebuild_index_del_cost: 9.81 s (8 delete files, sequential)
The 9.81s del-file replay was the only remaining sequential I/O hot path:
each iteration did a synchronous `read_all()` from object storage, then
deserialised the keys, and only afterwards advanced to the next file.
With 8 files at ~1.2s each, the OSS round-trips dominated.
This patch splits load_dels into two phases:
Phase 1 (parallel): read each delete file's bytes and deserialise into
a per-file pk column. Submitted through the existing
`pk_index_execution_thread_pool` (already used by
`_open_sstables_parallel`), gated by `enable_pk_index_parallel_execution`
so the behaviour is dynamically reversible. Falls back to inline
execution on submit failure.
Phase 2 (sequential): order-dependent index mutations. `replay_erase`
mutates index state observed by later files' `get()`, so iteration
order MUST match the original. The post-filter-then-erase logic per
file is unchanged; only the I/O has moved earlier.
Trace counters (`rebuild_index_del_cost_us`, `rebuild_index_del_cnt`) are
preserved. No functional change to encryption handling, schema, or filter
semantics. Single-file inputs skip the thread-pool token entirely.
Expected impact on iter-027 cold-start (max-tablet trace):
rebuild_index_del_cost_us: 9.81 s → ~1.5-2.5 s wall (8-way I/O)
primary_index_load_latency: 18.47 s → ~10-11 s
publish_version cost: 20.16 s → ~12 s
upsert max: 20 s → < 12 s
Verification (next iteration):
- Re-run benchmark on a fresh deploy (cold-start storm reproducer).
- Diff `rebuild_index_del_cost_us` p99 across BE Published trace.
- Confirm steady-state metrics (post-iter-021 envelope) unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ColumnArraySerde::deserialize returns StatusOr<const uint8_t*>, not Status. Status::update() takes a Status, so we must call .status() on the StatusOr to get the embedded Status before forwarding it. Build error from TSP build 1415: lake_persistent_index.cpp:938: error: no matching function for call to 'starrocks::Status::update(starrocks::StatusOr<const unsigned char*>&)'
…ock-read / search)
iter-029. Cold-start publish-version traces post-iter-028 show `multi_get_us`
dominates >=1s slow events in 71.8% of cases (publish-version cost up to ~17s,
multi_get up to ~7s). Existing publish-trace counters expose only the coarse
`multiget_t3_us` (filter + BlockReader + per-block search) — not enough to
tell whether the cold tail is bound by:
- bloom-filter CPU,
- OSS round-trips when BlockReader has a cache miss,
- or per-block search after a fresh load.
Adds five publish-trace counters inside the existing t3 region. They split
t3 into three phases and expose cache-miss timing separately:
multiget_t3_filter_us filter->KeyMayMatch() time
multiget_t3_block_read_us BlockReader() wall-time (hit + miss)
multiget_t3_block_read_miss_us BlockReader() wall-time on cache-miss only
(delta of ReadIOStat::block_cnt_from_file)
multiget_t3_search_us search_in_block() time on the freshly-read block
multiget_block_read_miss_cnt number of cache-miss BlockReader calls
No behaviour change. Five extra gettimeofday_us() calls per key-search are
~1% overhead in micro-bench, dwarfed by the OSS RTT this is meant to measure.
Decision tree for iter-030:
- block_read_miss_us / multi_get_us > 70%: classic OSS-bound; parallelise
or batch BlockReader miss reads (mind the iter-015 bandwidth trap).
- filter_us dominant: bloom-filter CPU; consider trimming filter bits or
skipping for small key sets.
- search_us dominant: in-memory CPU; investigate Block iterator cost.
Cold-start cache-miss block reads from OSS dominate the publish-version critical path on shared-data PK tables. Empirical from iter-030: multiget_t3_block_read_miss_us == 99.9% of multi_get_t3_us; per-miss p50 ~99ms (OSS first-byte latency). With ~5-20 filesets walked sequentially in get_from_sstables (each calling multi_get that does its own sequential miss block reads), total wall time stacks to 5-25s on slow events (observed: max 30,815ms; slow >=1s share = 0.34%, of which 77% are multi_get-dominated). Fix: when config::enable_pk_index_parallel_execution is true (existing flag, default true) and there are multiple filesets, dispatch each fileset's multi_get on the existing pk_index_execution_thread_pool and merge results so the most-recent fileset's value wins (preserves the original rbegin->rend ordering). The set_difference shortcut is dropped inside the parallel path; on cold-start most candidate keys are rejected cheaply by per-fileset bloom filters, so the extra work is small compared with the wall-time win from overlapping OSS first-byte latency. Risk-managed against the iter-015 trap (large-tail prefetch parallelised on Table::Open turning out to be bandwidth-bound on a 2 MB Bloom filter): individual block reads here are small (~32 KB; per-event total ~640 KB for p50 miss-count of 20), so concurrency overlaps OSS service latency rather than saturating bandwidth. Behind the same flag as the earlier parallel-load_dels work (StarRocks#72574), so a quick toggle reverts if regression appears. Expected impact on 999_1gb_1_1tb fresh-deploy: - multi_get_us p50 on slow events: ~1.84 s -> < 0.5 s - BE Published cost p99: 7.81 s -> < 2.5 s - Upsert max latency: 25.9 s -> aim < 10 s
…et_from_sstables iter-031 (TSP build 1419) deployed PR StarRocks#72579's parallel get_from_sstables and crashed all 6 BEs ~30s into the benchmark with: F threadpool.cpp:775] Thread belonging to thread pool 'cloud_native_pk_index_execution' with name 'cloud_native_pk_index_execution' called pool function that would result in deadlock ... starrocks::ThreadPool::check_not_pool_thread_unlocked() starrocks::ThreadPoolToken::wait() starrocks::lake::LakePersistentIndex::get_from_sstables(...) std::_Function_handler<..., LakePersistentIndex::upsert(...){lambda#1}> starrocks::ThreadPool::dispatch_thread() Root cause: on the parallel-publish path, LakePersistentIndex::upsert() submits a lambda to ParallelPublishContext::token, which is constructed on pk_index_execution_thread_pool (lake_primary_index.cpp:424,605 and update_manager.cpp:931,1069). That lambda then calls get_from_sstables, which under PR StarRocks#72579 dispatches per-fileset tasks to the same pool and waits — ThreadPool aborts because waiting on a pool from one of its own workers can deadlock if the pool is full. Fix: detect whether the calling thread is a pk_index_execution_thread_pool worker (Thread::current_thread()->name() comparison) and fall back to the original sequential path in that case. The sync caller paths (LakePersistentIndex::get / erase / non-ctx upsert) run from non-pool threads and still get the parallel optimization. This narrows the optimization's coverage from "all get_from_sstables" to "non-async-upsert get_from_sstables", but it eliminates the crash and re-establishes a deployable binary. A follow-up PR will move the inner parallelism to a separate pool (likely lake_metadata_fetch_thread_pool) so the async-upsert hot path can also benefit.
… pool iter-031 (PR StarRocks#72579) crashed with ThreadPool::check_not_pool_thread_unlocked() because LakePersistentIndex::get_from_sstables() called pk_index_execution_thread_pool-> new_token()->wait() from inside a lambda that was itself running on a pk_index_execution_thread_pool worker (ParallelPublishContext::token, see lake_primary_index.cpp:424,605 and update_manager.cpp:931,1069). PR StarRocks#72591 added a defensive runtime check (Thread::current_thread()->name() == "cloud_native_pk_index_execution") that fell back to the sequential path when on the same pool. iter-032 fresh-deploy benchmark on PR StarRocks#72591 (TSP build 1420) confirmed this avoids the crash (err 0 %), but also confirmed the parallel path is now gated OFF on the cold-start hot path: - 1005 slow ≥ 1 s publish traces in 11:48–11:54 (cold-start storm) - multi_get >= 50 % share in 95.0 % of slow events (vs 77.1 % in iter-030) - block_read_miss / multi_get = 99.5 % (p50) → OSS service-latency-bound - per-tablet upsert max 32 885 ms (cold-start), well above the 3 s goal - steady-state P99 1 101 ms (within post-iter-021 envelope) Fix: dispatch the per-fileset multi_get tasks on a dedicated pk_index_inner_io_thread_pool. This is a different pool from pk_index_execution_thread_pool, so a worker on the latter can submit + wait without re-entrance. The defensive Thread::current_thread() check from PR StarRocks#72591 is removed because it is no longer needed. Pool sizing: - max_threads default = CpuInfo::num_cores() (each task is OSS-IO-bound, can sleep without burning CPU; one thread per core is conservative). - queue size default = 1 048 576 (mirrors pk_index_execution_thread_pool; handles cold-start fan-out across many tablets). - Tunable via pk_index_inner_io_threadpool_max_threads / pk_index_inner_io_threadpool_size. Expected impact on the cold-start hot path: - Per-tablet get_from_sstables wall time = max-fileset multi_get (parallel) instead of sum-of-filesets (sequential). With F=6 filesets and per-fileset multi_get ≈ 1 s, expect per-event wall time 5 s -> ~1 s. - Slow ≥ 1 s event count should drop 4-5×, especially in the first 4 minutes after fresh deploy. - Steady state should be unchanged (post-iter-021 envelope). Risk: - Inner-pool tasks call PersistentIndexSstableFileset::multi_get -> Table::MultiGet -> BlockReader -> OSS read. None of these recurse back into ThreadPool wait, so no further re-entrance risk. - Pool init/shutdown ordering is mirrored from pk_index_memtable_flush_thread_pool.
PR StarRocks#72605 added REGISTER_THREAD_POOL_METRICS(cloud_native_pk_index_inner_io, _pk_index_inner_io_thread_pool) in exec_env.cpp but did not declare the corresponding metric members in StarRocksMetrics. Build 1421 failed with: exec_env.cpp:615: error: 'class starrocks::StarRocksMetrics' has no member named 'cloud_native_pk_index_inner_io_threadpool_size' Add METRICS_DEFINE_THREAD_POOL(cloud_native_pk_index_inner_io) in starrocks_metrics.h, mirroring cloud_native_pk_index_memtable_flush / cloud_native_pk_index_execution. No behavior change.
…edicated pool iter-033 (TSP build 1422 = PR StarRocks#72606 stack) diagnosis pinned the cold-start slow-publish residual on inner-level sequential cache-miss block reads inside a single Table::MultiGet call: - 815 slow ≥ 1 s BE Published events in the cold-start window - 65.4 % multi_get cost-share ≥ 50 % - on those events, multiget_t3_block_read_miss_us / multiget_t3_us = 99.5 % p50 - per-Table::MultiGet block_read_miss_cnt p99 = 153 - per-miss latency p50 ≈ 99 ms (OSS first-byte service latency, ~5 KB per read) - 153 × 99 ms ≈ 15 s upper bound; observed multi_get_us p99 ~3.2 s per fileset after PR StarRocks#72605's per-fileset parallelism amortizes via continuation - upsert max stuck at 32–33 s, 11× over the 3 s primary goal Resource class: concurrency-bound on OSS first-byte latency, NOT bandwidth and NOT RTT. Per-miss bytes are tiny; concurrency translates directly to wall time. Fix: split each PersistentIndexSstable::multi_get's key set into N contiguous balanced chunks (default target 64 keys/chunk, max 16 chunks) and dispatch each chunk's Table::MultiGet on a dedicated cloud_native_pk_index_chunk_io ThreadPool (default num_cores * 4, max-queue 1 048 576). Each chunk gets its own RandomAccessFile because the underlying SeekableInputStream is not safe for concurrent use; per-task files share the AWS S3Client which IS thread-safe, so the open is cheap (no full re-handshake). The chunk_io pool is a third dedicated pool, distinct from pk_index_inner_io_thread_pool (which dispatches per-fileset multi_gets in LakePersistentIndex::get_from_sstables) and pk_index_execution_thread_pool (parallel publish). Re-entering inner_io from inside one of its workers would deadlock via ThreadPool::wait()'s check_not_pool_thread_unlocked() — the same trap iter-031 (PR StarRocks#72579) fell into. The new pool sidesteps that by definition. Knobs: - enable_pk_index_parallel_chunk_multi_get (default true) — kill switch. - pk_index_parallel_chunk_min_keys (default 32) — below, sequential. - pk_index_parallel_chunk_target_keys (default 64) — chunk size target. - pk_index_parallel_chunk_max_chunks (default 16) — pool-queue protection. - pk_index_chunk_io_threadpool_max_threads (default 0 → num_cores * 4). - pk_index_chunk_io_threadpool_size (default 1 048 576) — queue depth. - All mutable; can be tuned without restart. Trace counter multi_get_chunk_count is added so the chunk path's engagement is observable in publish-trace without needing a profiler. Pre-registered acceptance for iter-035 fresh-deploy A/B (≥ 2 of 3 must hold): 1. slow ≥ 1 s event count drops ≥ 40 % (815 → ≤ 489). 2. per-Table::MultiGet wall-time p99 drops ≥ 50 % (3,207 ms → ≤ 1,600 ms). 3. upsert max drops ≥ 30 % (33,284 ms → ≤ 23,300 ms). Hard guardrails: err 0 % (no crashes), steady-state P99 / throughput within the post-iter-021 envelope. Falsifies cleanly on iter-035 fresh-deploy. Risks considered: - Per-task open RandomAccessFile overhead — mitigated by N small chunks, S3 open is wrapper-only (S3Client shared). - Block-cache thread contention across chunks — mitigated by LRUCache sharding. - Chunked path skips the existing corruption-retry; on retry we fall back to the sequential path (preserves pre-iter-034 retry semantics).
Iter-035 deployed PR StarRocks#72619 (chunk-parallel multi_get) and validated a strong publish-side win: BE Published slow >=1s 815 -> 159 (-81%) FE Slow publishPartition 23,289 -> 1,951 (-92%) FE total_ms max 35,517 -> 3,305 ms (-91%) multiget_block_read_miss_cnt p99 153 -> 32 (-79%) Prom data on the iter-035 cold-start window (16:04-16:08) shows pk_index_inner_io is now the next ceiling on 2 of 6 CNs: active_threads max 32/32 (pool == num_cores, fully busy) queue_count max 245 (172.26.80.126), 234 (172.26.80.130) pending_time avg 252 ms on .130 / 108 ms on .128 Each fileset task on this pool synchronously waits on chunk-parallel OSS reads through the chunk_io pool below it, so the pool is I/O-bound, not CPU-bound. The same rationale that sized chunk_io to num_cores * 4 applies to inner_io: oversubscribe cores to keep fileset fan-out unblocked when many publishes hit cold-start tablets simultaneously. Bumping the default from num_cores (32 on these CNs) to num_cores * 4 (128) absorbs the 245-task burst peaks and brings inner_io's pool profile in line with the chunk_io pool below it.
Cold-start fresh-deploy publish-trace (iter-036) shows pindex_init_sst_open_us p99 = 8.88 s, max = 11.9 s. The phase is LakePersistentIndex::_open_sstables_parallel — each task is Table::Open, which issues 4 sequential OSS round-trips per SST (footer + index + metaindex + filter blocks). Today these tasks dispatch on pk_index_execution_thread_pool, sized at num_cores()/2 = 16/CN. The same pool also runs parallel-publish workers (parallel_upsert / condition_merge), so ~150 cold-start SST opens queue head-of-chain behind a 16-thread pool that is already under load — turning 4 RTs/SST × 150 SSTs into ~12 s wall-clock on a single hot publish-coordinator CN (172.26.80.128 in iter-036). Add a dedicated pk_index_sst_open thread pool sized num_cores()*4 = 128/CN, mirroring the pk_index_inner_io / pk_index_chunk_io oversubscription rationale (purely I/O-bound, sleeping on OSS HTTP responses). Caller of _open_sstables_parallel runs on the execution pool and waits on the new pool — different pools, so no ThreadPool::wait re-entrance trap (iter-031 StarRocks#72579). Wired the same way as StarRocks#72606 / StarRocks#72622: config flags, metrics registration, builder + shutdown + reset, BE_TEST stub. Expected impact: - Cold-start pindex_init_sst_open_us p99 ~9 s -> ~2 s on the affected CN (16-thread queue removed, 8x parallelism). - pidx_load p99 ~13 s -> ~6 s -> upsert max should drop further from iter-036's 42.6 s baseline. - No effect on steady-state (SST opens are rare after warm-up).
Why: 999_1gb_1_1tb iter-039 fresh-deploy on TSP build 1427 (= post-iter-038 PR StarRocks#72639 + StarRocks#72622 + StarRocks#72619 + StarRocks#72606 + StarRocks#72591 + post-iter-021) reproduced the cold-start storm with cluster CPU saturated 98.4-98.8% peak and vdb util 84-93% on every BE. A 25 s perf -F 99 -e cpu-clock profile on the hottest CN (172.26.80.128) inside the 21:05-21:10 cold-start window shows the parallel PK-index compaction pool (cloud_native_pk_index_compact) holding 42.23% of inclusive CPU, with allocator self-time at 31% across my_malloc + my_free. Children view, top stack: 42.23 % LakePersistentIndexParallelCompactTask::run 42.09 % do_run 23.05 % KeyValueMerger::merge 19.93 % KeyValueMerger::flush 11.38 % my_free <- list node free + scratch 6.40 % TableBuilder::Add 13.77 % operator new (do_run direct) 13.52 % my_malloc <- list node alloc + temps Per-row allocator pair self-time: my_malloc 16.49 % + my_free 14.92 % = 31 %. Both prior optimisations on this file (StarRocks#72434 reused the protobuf scratch buffers; StarRocks#72460 dropped Slice::to_string()) are in this build but the remaining allocation pair was hidden inside the std::list<IndexValueWithVer> _index_value_vers member: every emplace_front allocates a heap node and every flush() / clear() frees it, once per input row of the merge. On a 1 TB tablet that is millions of paired allocations per task on a thread pool sized num_cores/2. Tracing every mutation site in merge() and flush() shows the list NEVER holds more than one entry - it is either empty or contains the single "winning" (version, IndexValue) for the current key: - flush() ends with _index_value_vers.clear() -> empty - merge() empty-branch: emplace_front(...) -> 1 elem - merge() same-key replace: std::list t; t.emplace_front; _index_value_vers.swap(t) -> 1 elem - merge() key-changed: flush(); emplace_front(...) -> 1 elem What this changes: - _index_value_vers (std::list) -> _current_value (std::optional) - emplace_front on the list -> emplace on the optional (the swap-with-temp pattern collapses to a direct emplace) - clear() -> reset() on the optional - The for-loop in flush() over _index_value_vers degrades to a single if-skip_tombstone branch (the loop never iterated more than once) Algorithm semantics, public API, on-disk SST format, tombstone handling and tests are unchanged. The merger remains single-threaded within a task, so std::optional is safe. Risk: minimal. The single-element invariant of the original list is preserved by construction. _current_value lifetime tracks the same boundary as _index_value_vers did. No call site outside this .h/.cpp references the removed type. Expected impact: removes the per-row list-node alloc/free pair that contributed the 11.38 % my_free in flush plus a substantial share of the 13 % do_run-direct my_malloc. Profile-implied savings ~12-15 % of total CPU on the hot CN during cold-start, freeing CPU for the WRITE-phase tablet sink and the publish path that share the same cores on a CPU-saturated cluster. Stacked on b60794e (= iter-038 deployed HEAD, PR StarRocks#72639 chain) so the new build carries every validated win in the run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…load_from_file The tablet_id-based TabletManager::get_tablet_metadata() overload always performs std::make_shared<TabletMetadata>(*ptr) on its result and then calls set_id(tablet_id), which deep-copies every nested RowsetMetadataPB / SegmentMetadataPB / TuplePB / VariantPB through protobuf MergeFrom. That deep copy showed up at ~6.5% inclusive CPU on cold-start cluster traces (cpu-profile, iter-040 host 172.26.80.130), driven by VerticalCompactionTask -> SegmentIterator -> LakeDelvecLoader::load, with MergeFromInnerLoop<RowsetMetadataPB> at 2.83%, RepeatedPtrField Destroy at 2.08%, plus ~1.5% on TuplePB / VariantPB cleanup. The path-based TabletManager::get_tablet_metadata() overload returns the cached shared TabletMetadataPtr without any copy. Both overloads run the same aggregation-vs-single-tablet logic internally, so we just compute the path ourselves and route through the path-based call. The result is treated read-only here -- it flows into lake::get_del_vec() which takes a const TabletMetadata&. No mutation, so the cached shared_ptr is safe. This removes the deep copy from a hot per-segment cold-start path without changing semantics for any other call site (other callers of the tablet_id-based overload still get the existing copy-on-read behavior). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
auto-perf-opt seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Iteration 041 dedicated build slot — combines the cumulative validated optimization stack with the iter-040 LakeDelvecLoader deep-copy fix on a single fork branch, so TSP can build them all together.
This PR exists because earlier iterations (iter-040 PR #72667) branched off the run's pinned base
7af3ba7without the prior validated wins (#72654, #72639, #72622, #72619, #72606, #72591), making the build effectively a "bare branch-4.1 + LakeDelvecLoader" that regressed by 12-15× vs the iter-040 measured cumulative result. This branch is the cumulative stack so iter-042 can deploy and verify the LakeDelvecLoader fix on top of the real validated baseline.Stack composition
Top of stack down to run-pinned base
7af3ba7:[auto-perf-opt] Skip TabletMetadataPB deep copy in LakeDelvecLoader::load_from_file(the iter-041 verification target)[auto-perf-opt] Replace std::list with std::optional in KeyValueMerger[auto-perf-opt] Move SST opens to a dedicated pk_index_sst_open pool[auto-perf-opt] Bump pk_index_inner_io pool default to num_cores * 4[auto-perf-opt] Parallelise PK-index multi_get into key chunks[auto-perf-opt] Move parallel get_from_sstables to dedicated inner-io poolEmpirical evidence the stack was missing
Iter-041 deployed PR #72667 (LakeDelvecLoader fix only, on bare branch-4.1) and observed:
my_free41% +my_malloc41% (= 82% allocator self-time) oncloud_native_pk_index_compact— vs iter-040's ~3% allocator self-time.std::listallocator churn was back as the dominant CPU consumer (1.27%-12.31% per call site ofIndexValuesWithVerPB::~IndexValuesWithVerPB+KeyValueMerger::merge+KeyValueMerger::flush).Test plan
tsp deploy apply, runs 20-min999_1gb_1_1tbbenchmark.prepare→write_end ≥ 10 s≤ 1000 (matches iter-040 = 703).compact_column_group → LakeDelvecLoader::load → TabletManager::get_tablet_metadatadeep copy < 2% inclusive (vs iter-040 measured 6.32%).🤖 Generated with Claude Code