HappenLee commented on code in PR #68498:
URL: https://github.com/apache/doris/pull/68498#discussion_r4130215920
##########
be/src/core/value/quantile_state.cpp:
##########
@@ -28,7 +30,50 @@
#include "util/tdigest.h"
#include "util/unaligned.h"
+#ifdef BE_TEST
+#include "cpp/sync_point.h"
+#endif
+
namespace doris {
+
+// Shares a digest across QuantileState copies and detaches it before sample
+// writes. Readers use shared locks; compression uses an exclusive lock.
+struct QuantileState::TDigestHolder {
+ explicit TDigestHolder(float compression) : digest(compression) {}
+ TDigestHolder(const TDigestHolder& other) : digest(other.digest) {}
+
+ std::shared_lock<std::shared_mutex> lock_processed_digest() {
+ std::shared_lock read_lock(mutex);
+ if (digest.have_unprocessed()) {
+ read_lock.unlock();
+ {
+ std::unique_lock write_lock(mutex);
+ if (digest.have_unprocessed()) {
+ digest.compress();
+ }
+ }
+ read_lock.lock();
+ }
+ return read_lock;
+ }
+
+ TDigest digest;
+ std::shared_mutex mutex;
+};
+
+TDigest& QuantileState::_mutable_tdigest() {
+ std::shared_ptr<TDigestHolder> detached;
+ {
+ std::unique_lock lock(_tdigest_ptr->mutex);
+ if (_tdigest_ptr.use_count() == 1) {
+ return _tdigest_ptr->digest;
+ }
+ detached = std::make_shared<TDigestHolder>(*_tdigest_ptr);
Review Comment:
Suggested minimal change for this finding: replace the capacity-preserving
TDigest copy constructor with `TDigest(const TDigest&) = default`, or an
equivalent member-wise copy that copies the vectors' live elements without
explicitly reserving the source capacities or the maximum write-buffer sizes.
Keep `_max_processed` and `_max_unprocessed` unchanged: they are algorithm
thresholds, not allocation sizes.
The current `_unprocessed.reserve(max(other.capacity(), _max_unprocessed +
1))` allocates at least 80001 centroids at compression 10000 even when the
source's unprocessed vector is empty. A growing `quantile_union(q)` window
saves the accumulator after each row; the following merge detaches, so
successive result holders retain these buffers. About 1000 such buffers account
for 610 MiB of allocation capacity, excluding the other vectors. This is a
code-derived capacity estimate, not a measured RSS result.
Please update `CopyPreservesValuesAndWriteCapacity` to check
values/isolation and avoidance of unnecessary spare capacity, and add a long
high-compression window capacity test. Member-wise copying removes this
unconditional reservation, but subsequent growth and existing saved-holder
capacity still need checking. The existing omission of internal buffers from
`ColumnComplexType::allocated_bytes()` also makes block-level accounting
underestimate this cost.
##########
be/src/core/value/quantile_state.cpp:
##########
@@ -28,7 +30,50 @@
#include "util/tdigest.h"
#include "util/unaligned.h"
+#ifdef BE_TEST
+#include "cpp/sync_point.h"
+#endif
+
namespace doris {
+
+// Shares a digest across QuantileState copies and detaches it before sample
+// writes. Readers use shared locks; compression uses an exclusive lock.
+struct QuantileState::TDigestHolder {
+ explicit TDigestHolder(float compression) : digest(compression) {}
+ TDigestHolder(const TDigestHolder& other) : digest(other.digest) {}
+
+ std::shared_lock<std::shared_mutex> lock_processed_digest() {
+ std::shared_lock read_lock(mutex);
+ if (digest.have_unprocessed()) {
+ read_lock.unlock();
+ {
+ std::unique_lock write_lock(mutex);
+ if (digest.have_unprocessed()) {
+ digest.compress();
+ }
+ }
+ read_lock.lock();
+ }
+ return read_lock;
+ }
+
+ TDigest digest;
+ std::shared_mutex mutex;
+};
+
+TDigest& QuantileState::_mutable_tdigest() {
+ std::shared_ptr<TDigestHolder> detached;
+ {
+ std::unique_lock lock(_tdigest_ptr->mutex);
Review Comment:
Performance suggestion: could the ownership check and source cloning use a
`std::shared_lock` here instead of a `std::unique_lock`? The detached-holder
construction only reads the source digest, but currently holds the exclusive
lock across allocation and all vector copies. Consequently, independent copies
detaching from the same holder serialize their cloning and block otherwise
read-only percentile queries and source merges.
A shared lock would still exclude `lock_processed_digest()`'s exclusive
compression phase while allowing source readers and other clones to overlap.
This needs to preserve the existing contract that mutation of the same outer
QuantileState requires exclusive access; sample writes must still happen only
after obtaining an exclusively owned digest, and replacement of the old holder
must remain outside the lock's lifetime.
Please evaluate this with a synchronized concurrent-detach/read test and a
shared-source benchmark. This is an optimization opportunity identified from
the lock scope, not a separately demonstrated correctness bug or a measured
performance regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
