HappenLee commented on PR #70421:
URL: https://github.com/apache/doris/pull/70421#issuecomment-6788464100
Potential performance regression: validation can clone columns that are
already exclusively owned.
The shared-column isolation is needed for correctness, but the new helper
introduces two extra owners before the COW decision in
`_internal_validate_column()`:
```cpp
ColumnPtr real_column_ptr =
column_ptr == nullptr ? column : column_ptr->get_nested_column_ptr();
// Inside mutate_column():
auto mutable_column = IColumn::mutate(column);
```
For an initially exclusive, non-nullable column, the reference count is 1 in
the owning slot, 2 after constructing `real_column_ptr`, and 3 when the lvalue
`column` is copied into the by-value parameter of `IColumn::mutate(Ptr ptr)`.
Consequently, `shallow_mutate()` takes the `clone()` branch even though there
is no other consumer that needs the old column. The `ColumnPtr&` validation
parameter itself does not add an owner.
```cpp
static MutablePtr mutate(Ptr ptr) {
MutablePtr res = ptr->shallow_mutate(); // Clone decision happens here.
ptr.reset(); // Too late to avoid that clone.
res->mutate_subcolumns();
return res;
}
```
For nullable columns, the local alias additionally retains the nested column
while the by-value parameter retains the wrapper. Also, `mutate_subcolumns()`
recursively detaches descendants. The ARRAY/MAP/STRUCT validation branches then
recurse and repeat the same ownership pattern, potentially copying
already-detached descendants again. Even read-only validation of an exclusively
owned `ARRAY<INT>` enters `mutate_column()` eagerly.
This can add column-data copying, allocation, memory bandwidth use, and peak
memory, especially for large or deeply nested complex columns. Copying the
smart pointers only increments reference counts; the subsequent COW calls cause
the column clones.
Suggested changes:
1. Use non-owning, read-only views (for example `const IColumn*`) for
`real_column_ptr`, and refresh them after replacing the owning column.
2. Acquire mutable ownership by consuming the actual owner through the
existing scoped ownership mechanism where applicable, preserving restoration on
early returns and exceptions. Changing only the call to
`IColumn::mutate(std::move(column))` is insufficient while the owning
`real_column_ptr` alias is still alive.
3. Once a column tree has been made mutable, avoid repeating whole-subtree
COW at each recursive validation level, and defer mutation for paths that only
need to inspect values. Keep the necessary COW isolation for genuinely shared
parents and descendants.
Please add ownership/allocation coverage alongside the SQL correctness
regression: exclusive columns and descendants should be reused when possible;
shared outputs must remain isolated during string truncation; nested
ARRAY/MAP/STRUCT inputs should not repeatedly clone the same already-detached
payload.
Validation: traced the COW implementation and reproduced the extra-clone
pattern in an isolated model using its reference-counting implementation. This
is not a full BE benchmark, so no SQL slowdown or speedup percentage is claimed.
--
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]
