Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Fix deprecated errors when using shared_ptr::unique() #11318

Closed
wants to merge 1 commit into from
Closed

refactor: Fix deprecated errors when using shared_ptr::unique() #11318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion velox/dwio/common/SelectiveRepeatedColumnReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ void prepareResult(
result->encoding() == VectorEncoding::Simple::ARRAY) ||
(type->kind() == TypeKind::MAP &&
result->encoding() == VectorEncoding::Simple::MAP)) &&
result.unique())) {
result.use_count() == 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether we should even rely on use_count as its unreliable , see for e.g : https://stackoverflow.com/a/77286276 .

Copy link
Collaborator Author

@czentgr czentgr Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it is unreliable. This change is a 1-1 replacement for now. Using unique was already unreliable and I guess that is why it is being removed. It will need some change eventually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood , I will create another issue to remove the use of shared_ptr::use_count == 1 from the code base as its unreliable and not meant for use across threads.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, was about to create this issue :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#11344

VLOG(1) << "Reallocating result " << type->kind() << " vector of size "
<< size;
result = BaseVector::create(type, size, pool);
Expand Down
8 changes: 4 additions & 4 deletions velox/dwio/common/SelectiveStructColumnReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ void fillRowVectorChildren(
}

VectorPtr tryReuseResult(const VectorPtr& result) {
if (!result.unique()) {
if (result.use_count() != 1) {
return nullptr;
}
switch (result->encoding()) {
Expand Down Expand Up @@ -99,7 +99,7 @@ void setConstantField(
const VectorPtr& constant,
vector_size_t size,
VectorPtr& field) {
if (field && field->isConstantEncoding() && field.unique() &&
if (field && field->isConstantEncoding() && field.use_count() == 1 &&
field->size() > 0 && field->equalValueAt(constant.get(), 0, 0)) {
field->resize(size);
} else {
Expand All @@ -112,7 +112,7 @@ void setNullField(
VectorPtr& field,
const TypePtr& type,
memory::MemoryPool* pool) {
if (field && field->isConstantEncoding() && field.unique() &&
if (field && field->isConstantEncoding() && field.use_count() == 1 &&
field->size() > 0 && field->isNullAt(0)) {
field->resize(size);
} else {
Expand Down Expand Up @@ -512,7 +512,7 @@ void SelectiveStructColumnReaderBase::getValues(
}
auto lazyLoader =
std::make_unique<ColumnLoader>(this, children_[index], numReads_);
if (childResult && childResult->isLazy() && childResult.unique()) {
if (childResult && childResult->isLazy() && childResult.use_count() == 1) {
majetideepak marked this conversation as resolved.
Show resolved Hide resolved
static_cast<LazyVector&>(*childResult)
.reset(std::move(lazyLoader), rows.size());
} else {
Expand Down
2 changes: 1 addition & 1 deletion velox/dwio/common/SelectiveStructColumnReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ class SelectiveFlatMapColumnReaderHelper {
private:
MapVector& prepareResult(VectorPtr& result, vector_size_t size) {
if (result && result->encoding() == VectorEncoding::Simple::MAP &&
result.unique()) {
result.use_count() == 1) {
result->resetDataDependentFlags(nullptr);
result->resize(size);
} else {
Expand Down
2 changes: 1 addition & 1 deletion velox/exec/AssignUniqueId.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ bool AssignUniqueId::isFinished() {
void AssignUniqueId::generateIdColumn(vector_size_t size) {
// Re-use memory for the ID vector if possible.
VectorPtr& result = results_[0];
if (result && result.unique()) {
if (result && result.use_count() == 1) {
BaseVector::prepareForReuse(result, size);
} else {
result = BaseVector::create(BIGINT(), size, pool());
Expand Down
4 changes: 2 additions & 2 deletions velox/exec/GroupingSet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,7 +1291,7 @@ namespace {
// Recursive resize all children.

void recursiveResizeChildren(VectorPtr& vector, vector_size_t newSize) {
VELOX_CHECK(vector.unique());
VELOX_CHECK_EQ(vector.use_count(), 1);
if (vector->typeKind() == TypeKind::ROW) {
auto rowVector = vector->asUnchecked<RowVector>();
for (auto& child : rowVector->children()) {
Expand All @@ -1307,7 +1307,7 @@ void GroupingSet::toIntermediate(
const RowVectorPtr& input,
RowVectorPtr& result) {
VELOX_CHECK(abandonedPartialAggregation_);
VELOX_CHECK(result.unique());
VELOX_CHECK_EQ(result.use_count(), 1);
if (!isRawInput_) {
result = input;
return;
Expand Down
4 changes: 2 additions & 2 deletions velox/exec/HashProbe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ RowVectorPtr HashProbe::getBuildSideOutput() {
}

void HashProbe::clearIdentityProjectedOutput() {
if (!output_ || !output_.unique()) {
if (!output_ || output_.use_count() != 1) {
return;
}
for (auto& projection : identityProjections_) {
Expand Down Expand Up @@ -1212,7 +1212,7 @@ const uint64_t* getFlatFilterResult(VectorPtr& result) {
if (!flat->rawValues<uint64_t>()) {
return flat->rawNulls();
}
if (!result.unique()) {
if (result.use_count() != 1) {
return nullptr;
}
auto* values = flat->mutableRawValues<uint64_t>();
Expand Down
2 changes: 1 addition & 1 deletion velox/exec/MarkDistinct.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ RowVectorPtr MarkDistinct::getOutput() {
auto outputSize = input_->size();
// Re-use memory for the ID vector if possible.
VectorPtr& result = results_[0];
if (result && result.unique()) {
if (result && result.use_count() == 1) {
BaseVector::prepareForReuse(result, outputSize);
} else {
result = BaseVector::create(BOOLEAN(), outputSize, operatorCtx_->pool());
Expand Down
2 changes: 1 addition & 1 deletion velox/exec/OutputBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ void OutputBuffer::updateAfterAcknowledgeLocked(
uint64_t freedBytes{0};
int freedPages{0};
for (const auto& free : freed) {
if (free.unique()) {
if (free.use_count() == 1) {
++freedPages;
freedBytes += free->size();
}
Expand Down
2 changes: 1 addition & 1 deletion velox/exec/RowNumber.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ void RowNumber::ensureInputFits(const RowVectorPtr& input) {

FlatVector<int64_t>& RowNumber::getOrCreateRowNumberVector(vector_size_t size) {
VectorPtr& result = results_[0];
if (result && result.unique()) {
if (result && result.use_count() == 1) {
BaseVector::prepareForReuse(result, size);
} else {
result = BaseVector::create(BIGINT(), size, pool());
Expand Down
4 changes: 2 additions & 2 deletions velox/expression/ConstantExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ void ConstantExpr::evalSpecialForm(
const SelectivityVector& rows,
EvalCtx& context,
VectorPtr& result) {
if (sharedConstantValue_.unique()) {
if (sharedConstantValue_.use_count() == 1) {
sharedConstantValue_->resize(rows.end());
} else {
// By reassigning sharedConstantValue_ we increase the chances that it will
Expand All @@ -33,7 +33,7 @@ void ConstantExpr::evalSpecialForm(
if (needToSetIsAscii_) {
// sharedConstantValue_ must be unique because computeAndSetIsAscii may
// modify it.
VELOX_CHECK(sharedConstantValue_.unique());
VELOX_CHECK_EQ(sharedConstantValue_.use_count(), 1);
auto* vector =
sharedConstantValue_->asUnchecked<SimpleVector<StringView>>();
LocalSingleRow singleRow(context, 0);
Expand Down
4 changes: 2 additions & 2 deletions velox/expression/EvalCtx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ void EvalCtx::addNulls(
// or do nothing otherwise.
if (result->isConstantEncoding() && result->isNullAt(0)) {
if (result->size() < rows.end()) {
if (result.unique()) {
if (result.use_count() == 1) {
result->resize(rows.end());
} else {
result =
Expand All @@ -389,7 +389,7 @@ void EvalCtx::addNulls(

auto currentSize = result->size();
auto targetSize = rows.end();
if (!result.unique() || !result->isNullsWritable()) {
if (result.use_count() != 1 || !result->isNullsWritable()) {
if (result->type()->isPrimitiveType()) {
if (currentSize < targetSize) {
resizePrimitiveTypeVectors(result, targetSize, context);
Expand Down
2 changes: 1 addition & 1 deletion velox/expression/SimpleFunctionAdapter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ class SimpleFunctionAdapter : public VectorFunction {
!providesCustomComparison<arg_at<POSITION>>::value) {
using type =
typename VectorExec::template resolver<arg_at<POSITION>>::in_type;
if (args[POSITION]->isFlatEncoding() && args[POSITION].unique() &&
if (args[POSITION]->isFlatEncoding() && args[POSITION].use_count() == 1 &&
args[POSITION]->asUnchecked<FlatVector<type>>()->isWritable()) {
// Re-use arg for result. We rely on the fact that for each row
// we read arguments before computing and writing out the
Expand Down
2 changes: 1 addition & 1 deletion velox/expression/TryExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ void TryExpr::nullOutErrors(
auto indices = allocateIndices(size, context.pool());
result = BaseVector::wrapInDictionary(nulls, indices, size, result);
} else if (
result.unique() && result->isNullsWritable() &&
result.use_count() == 1 && result->isNullsWritable() &&
result->size() >= rows.end()) {
auto* rawNulls = result->mutableRawNulls();
rows.applyToSelected([&](auto row) {
Expand Down
10 changes: 5 additions & 5 deletions velox/expression/tests/ExprTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2114,14 +2114,14 @@ TEST_F(ExprTest, memo) {
100, [](auto row) { return (8 + row * 2) % 3 == 1; });
assertEqualVectors(expectedResult, result);
VELOX_CHECK_EQ(stats["eq"].numProcessedRows, 100);
VELOX_CHECK(base.unique());
VELOX_CHECK_EQ(base.use_count(), 1);

// After this results would be cached
std::tie(result, stats) = evaluateWithStats(
exprSet.get(), makeRowVector({wrapInDictionary(evenIndices, 100, base)}));
assertEqualVectors(expectedResult, result);
VELOX_CHECK_EQ(stats["eq"].numProcessedRows, 200);
VELOX_CHECK(!base.unique());
VELOX_CHECK_NE(base.use_count(), 1);

// Unevaluated rows are processed
std::tie(result, stats) = evaluateWithStats(
Expand All @@ -2130,7 +2130,7 @@ TEST_F(ExprTest, memo) {
100, [](auto row) { return (9 + row * 2) % 3 == 1; });
assertEqualVectors(expectedResult, result);
VELOX_CHECK_EQ(stats["eq"].numProcessedRows, 300);
VELOX_CHECK(!base.unique());
VELOX_CHECK_NE(base.use_count(), 1);

auto everyFifth = makeIndices(100, [](auto row) { return row * 5; });
std::tie(result, stats) = evaluateWithStats(
Expand All @@ -2142,7 +2142,7 @@ TEST_F(ExprTest, memo) {
stats["eq"].numProcessedRows,
360,
"Fewer rows expected as memoization should have kicked in.");
VELOX_CHECK(!base.unique());
VELOX_CHECK_NE(base.use_count(), 1);

// Create a new base
base = makeArrayVector<int64_t>(
Expand All @@ -2156,7 +2156,7 @@ TEST_F(ExprTest, memo) {
100, [](auto row) { return (9 + row * 2) % 3 == 1; });
assertEqualVectors(expectedResult, result);
VELOX_CHECK_EQ(stats["eq"].numProcessedRows, 460);
VELOX_CHECK(base.unique());
VELOX_CHECK_EQ(base.use_count(), 1);
}

// This test triggers the situation when peelEncodings() produces an empty
Expand Down
6 changes: 4 additions & 2 deletions velox/functions/prestosql/VectorArithmetic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,13 @@ class VectorArithmetic : public VectorFunction {
// (2) the input type must match the output vector type
// (3) usually we try to reuse inputs with flat encoding
if (!result) {
if (args[0].unique() && leftEncoding == VectorEncoding::Simple::FLAT) {
if (args[0].use_count() == 1 &&
leftEncoding == VectorEncoding::Simple::FLAT) {
result = std::move(args[0]);
} else if (

args[1].unique() && rightEncoding == VectorEncoding::Simple::FLAT) {
args[1].use_count() == 1 &&
rightEncoding == VectorEncoding::Simple::FLAT) {
result = std::move(args[1]);
} else {
result = BaseVector::create(outputType, rows.end(), context.pool());
Expand Down
6 changes: 3 additions & 3 deletions velox/serializers/PrestoSerializer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4201,9 +4201,9 @@ void PrestoVectorSerde::deserialize(

if (resultOffset > 0) {
VELOX_CHECK_NOT_NULL(*result);
VELOX_CHECK(result->unique());
VELOX_CHECK_EQ(result->use_count(), 1);
(*result)->resize(resultOffset + header.numRows);
} else if (*result && result->unique()) {
} else if (*result && result->use_count() == 1) {
VELOX_CHECK(
*(*result)->type() == *type,
"Unexpected type: {} vs. {}",
Expand Down Expand Up @@ -4245,7 +4245,7 @@ void PrestoVectorSerde::deserializeSingleColumn(
VELOX_CHECK_EQ(
prestoOptions.compressionKind,
common::CompressionKind::CompressionKind_NONE);
if (*result && result->unique()) {
if (*result && result->use_count() == 1) {
VELOX_CHECK(
*(*result)->type() == *type,
"Unexpected type: {} vs. {}",
Expand Down
4 changes: 2 additions & 2 deletions velox/vector/BaseVector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ void BaseVector::ensureWritable(
if (result->encoding() == VectorEncoding::Simple::LAZY) {
result = BaseVector::loadedVectorShared(result);
}
if (result.unique() && !isUnknownType) {
if (result.use_count() == 1 && !isUnknownType) {
switch (result->encoding()) {
case VectorEncoding::Simple::FLAT:
case VectorEncoding::Simple::ROW:
Expand Down Expand Up @@ -879,7 +879,7 @@ void BaseVector::flattenVector(VectorPtr& vector) {
}

void BaseVector::prepareForReuse(VectorPtr& vector, vector_size_t size) {
if (!vector.unique() || !isReusableEncoding(vector->encoding())) {
if (vector.use_count() != 1 || !isReusableEncoding(vector->encoding())) {
vector = BaseVector::create(vector->type(), size, vector->pool());
return;
}
Expand Down
2 changes: 1 addition & 1 deletion velox/vector/BaseVector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ class BaseVector {
/// unique.
template <typename T>
static bool isVectorWritable(const std::shared_ptr<T>& vector) {
if (!vector.unique()) {
if (vector.use_count() != 1) {
return false;
}

Expand Down
4 changes: 2 additions & 2 deletions velox/vector/ComplexVector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,7 @@ void RowVector::resize(vector_size_t newSize, bool setNotNull) {
// to skip uniqueness check since effectively we are just changing
// the length.
if (newSize > oldSize) {
VELOX_CHECK(child.unique(), "Resizing shared child vector");
VELOX_CHECK_EQ(child.use_count(), 1, "Resizing shared child vector");
child->resize(newSize, setNotNull);
}
}
Expand Down Expand Up @@ -1328,7 +1328,7 @@ void MapVector::canonicalize(
// threads. The keys and values do not have to be uniquely owned
// since they are not mutated but rather transposed, which is
// non-destructive.
VELOX_CHECK(map.unique());
VELOX_CHECK(map.use_count() == 1);
BufferPtr indices;
vector_size_t* indicesRange;
for (auto i = 0; i < map->BaseVector::length_; ++i) {
Expand Down
2 changes: 1 addition & 1 deletion velox/vector/VectorPool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ bool VectorPool::release(VectorPtr& vector) {
if (FOLLY_UNLIKELY(vector == nullptr)) {
return false;
}
if (!vector.unique() || vector->size() > kMaxRecycleSize) {
if (vector.use_count() != 1 || vector->size() > kMaxRecycleSize) {
return false;
}

Expand Down
16 changes: 8 additions & 8 deletions velox/vector/tests/EnsureWritableVectorTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ struct VectorPointersBase {
}

void assertUnique(const VectorPtr& vector) {
ASSERT_TRUE(vector.unique());
ASSERT_EQ(vector.use_count(), 1);

auto* typedVector = vector->as<T>();
ASSERT_EQ(nullsUnique, typedVector->nulls()->unique());
Expand Down Expand Up @@ -299,7 +299,7 @@ struct ArrayVectorPointers : public VectorPointersBase<ArrayVector> {
VectorPointersBase<ArrayVector>::assertUnique(vector);

auto* arrayVector = vector->as<ArrayVector>();
ASSERT_EQ(elementsUnique, arrayVector->elements().unique());
ASSERT_EQ(elementsUnique, (arrayVector->elements().use_count() == 1));
}

void assertPointers(const VectorPtr& vector) {
Expand Down Expand Up @@ -338,8 +338,8 @@ struct MapVectorPointers : public VectorPointersBase<MapVector> {
VectorPointersBase<MapVector>::assertUnique(vector);

auto* mapVector = vector->as<MapVector>();
ASSERT_EQ(keysUnique, mapVector->mapKeys().unique());
ASSERT_EQ(valuesUnique, mapVector->mapValues().unique());
ASSERT_EQ(keysUnique, (mapVector->mapKeys().use_count() == 1));
ASSERT_EQ(valuesUnique, (mapVector->mapValues().use_count() == 1));
}

void assertPointers(const VectorPtr& vector) {
Expand Down Expand Up @@ -476,11 +476,11 @@ TEST_F(EnsureWritableVectorTest, array) {

// Multiply-referenced vector
auto resultCopy = result;
ASSERT_FALSE(result.unique());
ASSERT_FALSE(result.use_count() == 1);

auto oddRows = selectOddRows(size);
BaseVector::ensureWritable(oddRows, result->type(), pool(), result);
ASSERT_TRUE(result.unique());
ASSERT_TRUE(result.use_count() == 1);
ASSERT_NE(resultCopy.get(), result.get());

// Verify that even rows were copied over
Expand Down Expand Up @@ -601,11 +601,11 @@ TEST_F(EnsureWritableVectorTest, map) {

// Multiply-referenced vector
auto resultCopy = result;
ASSERT_FALSE(result.unique());
ASSERT_FALSE(result.use_count() == 1);

auto oddRows = selectOddRows(size);
BaseVector::ensureWritable(oddRows, result->type(), pool(), result);
ASSERT_TRUE(result.unique());
ASSERT_TRUE(result.use_count() == 1);
ASSERT_NE(resultCopy.get(), result.get());

// Verify that even rows were copied over
Expand Down
6 changes: 3 additions & 3 deletions velox/vector/tests/VectorTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1549,7 +1549,7 @@ TEST_F(VectorTest, wrapInConstantWithCopy) {
std::dynamic_pointer_cast<ConstantVector<ComplexType>>(
BaseVector::wrapInConstant(size, 22, constBaseVector, true));
EXPECT_NE(constArrayVector->valueVector(), nullptr);
EXPECT_TRUE(constArrayVector->valueVector().unique());
EXPECT_EQ(constArrayVector->valueVector().use_count(), 1);
for (auto i = 0; i < size; i++) {
ASSERT_FALSE(constArrayVector->isNullAt(i));
ASSERT_TRUE(constArrayVector->equalValueAt(arrayVector.get(), i, 3));
Expand All @@ -1561,7 +1561,7 @@ TEST_F(VectorTest, wrapInConstantWithCopy) {
constArrayVector = std::dynamic_pointer_cast<ConstantVector<ComplexType>>(
BaseVector::wrapInConstant(size, 22, constBaseVector, true));
EXPECT_NE(constArrayVector->valueVector(), nullptr);
EXPECT_TRUE(constArrayVector->valueVector().unique());
EXPECT_EQ(constArrayVector->valueVector().use_count(), 1);
for (auto i = 0; i < size; i++) {
ASSERT_TRUE(constArrayVector->isNullAt(i));
}
Expand All @@ -1571,7 +1571,7 @@ TEST_F(VectorTest, rowResize) {
auto testRowResize = [&](const VectorPtr& vector, bool setNotNull) {
auto rowVector = vector->as<RowVector>();
for (auto& child : rowVector->children()) {
VELOX_CHECK(child.unique());
VELOX_CHECK_EQ(child.use_count(), 1);
}
auto oldSize = rowVector->size();
auto newSize = oldSize * 2;
Expand Down
Loading