Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 31 additions & 7 deletions tree/ntuple/src/RFieldSequenceContainer.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -282,26 +282,50 @@ unsigned char *ROOT::RRVecField::ResizeRVec(void *rvec, std::size_t nItems, std:
}

// Resize RVec (capacity and size)
if (std::int32_t(nItems) > *capacityPtr) { // must reallocate
// In the regime between 0 and 16 items, grow by doubling the size. Grow larger vectors by a factor 1.5 at most.
// If the new size is larger than the grown capacity, set the new capacity to the new size (i.e., don't allocate
// a larger capacity for vectors the are suddenly vastly larger than their previous state).
// TODO(jblomer): investigate a longer look back window to avoid shrinking for an unusual small entry and consider
// returning to average or median size (see TBasket::WriteReset()).
static constexpr std::int32_t kSmallThreshold = 16;
if ((std::int32_t(nItems) > *capacityPtr) || /* must grow */
((std::int32_t(nItems) > kSmallThreshold) && (std::int32_t(nItems) < *capacityPtr / 4))) /* must shrink */ {
Comment thread
pcanal marked this conversation as resolved.
// Destroy old elements: useless work for trivial types, but in case the element type's constructor
// allocates memory we need to release it here to avoid memleaks (e.g. if this is an RVec<RVec<int>>)
if (needsDestruct) {
for (std::size_t i = 0u; i < oldSize; ++i) {
// Excess elements were destructed earlier.
for (std::size_t i = 0u; i < std::min(oldSize, nItems); ++i) {
Comment thread
jblomer marked this conversation as resolved.
itemDeleter->operator()(*beginPtr + (i * itemSize), true /* dtorOnly */);
}
}

// TODO Increment capacity by a factor rather than just enough to fit the elements.
// Increment / decrement capacity geometrically
std::size_t newCapacity = nItems;
if (*capacityPtr > 0) {
constexpr std::size_t maxSize = static_cast<std::size_t>(std::numeric_limits<std::int32_t>::max());
const std::size_t oldCap = static_cast<std::size_t>(*capacityPtr);
if (nItems > oldCap) {
const std::size_t grown = oldCap + ((oldCap < kSmallThreshold) ? oldCap : (oldCap / 2));
newCapacity = std::min(maxSize, std::max(nItems, grown));
} else {
newCapacity = oldCap / 2;
assert(nItems <= newCapacity);
}
}

Internal::DestroyRVecWithChecks(itemField->GetAlignment(), beginPtr, capacityPtr);
// We trust that malloc returns a buffer with large enough alignment.
// This might not be the case if T in RVec<T> is over-aligned.
*beginPtr = static_cast<unsigned char *>(malloc(nItems * itemSize));
*beginPtr = static_cast<unsigned char *>(malloc(newCapacity * itemSize));
R__ASSERT(*beginPtr != nullptr);
*capacityPtr = nItems;
*capacityPtr = static_cast<std::int32_t>(newCapacity);

// Placement new for elements that were already there before the resize
// Placement new for elements that were already there before the resize. When shrank, we only need to construct
// nItems items, because that's what we going to read. When grown, the remaining elements from oldSize until
// nItems is constructed a few lines down (we need to do this always when the size grows, even when not changing
// the capacity).
if (needsConstruct) {
for (std::size_t i = 0u; i < oldSize; ++i)
for (std::size_t i = 0u; i < std::min(oldSize, nItems); ++i)
CallConstructValueOn(*itemField, *beginPtr + (i * itemSize));
}
}
Expand Down
123 changes: 113 additions & 10 deletions tree/ntuple/test/rfield_vector.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ TEST(RNTuple, InsideCollection)
// TODO: test reading of "klassVec.v1"
}


TEST(RNTuple, RVec)
{
FileRaii fileGuard("test_ntuple_rvec.root");
Expand Down Expand Up @@ -304,6 +303,107 @@ TEST(RNTuple, ReadVectorAsRVec)
EXPECT_TRUE(rvec->empty());
}

TEST(RNTuple, RVecGeometricResize)
{
FileRaii fileGuard("test_ntuple_rvec_geometric_resize.root");

{
auto m = RNTupleModel::Create();
auto rvec = m->MakeField<ROOT::RVec<int>>("v");
auto w = RNTupleWriter::Recreate(std::move(m), "ntpl", fileGuard.GetPath());

for (int i = 0; i < 6; ++i) {
w->Fill();
rvec->push_back(i);
}

*rvec = {10};
w->Fill();

*rvec = ROOT::RVec(11, 0);
w->Fill();

*rvec = ROOT::RVec(23, 0);
w->Fill();

rvec->clear();
w->Fill();

*rvec = ROOT::RVec(100, 0);
w->Fill();

*rvec = ROOT::RVec(101, 0);
w->Fill();

*rvec = {1, 2};
w->Fill();

*rvec = {1, 2, 3};
w->Fill();
}

auto m = RNTupleModel::Create();
auto rvec = m->MakeField<ROOT::RVec<int>>("v");
auto reader = RNTupleReader::Open(std::move(m), "ntpl", fileGuard.GetPath());
EXPECT_EQ(14u, reader->GetNEntries());

reader->LoadEntry(0);
EXPECT_EQ(rvec->size(), 0u);
EXPECT_EQ(rvec->capacity(), 0u);

reader->LoadEntry(1);
EXPECT_EQ(rvec->size(), 1u);
EXPECT_EQ(rvec->capacity(), 1u);

reader->LoadEntry(2);
EXPECT_EQ(rvec->size(), 2u);
EXPECT_EQ(rvec->capacity(), 2u);

reader->LoadEntry(3);
EXPECT_EQ(rvec->size(), 3u);
EXPECT_EQ(rvec->capacity(), 4u);

reader->LoadEntry(4);
EXPECT_EQ(rvec->size(), 4u);
EXPECT_EQ(rvec->capacity(), 4u);

reader->LoadEntry(5);
EXPECT_EQ(rvec->size(), 5u);
EXPECT_EQ(rvec->capacity(), 8u);

reader->LoadEntry(6);
EXPECT_EQ(rvec->size(), 1u);
EXPECT_GE(rvec->capacity(), 8u);

reader->LoadEntry(7);
EXPECT_EQ(rvec->size(), 11u);
EXPECT_GE(rvec->capacity(), 16u);

reader->LoadEntry(8);
EXPECT_EQ(rvec->size(), 23u);
EXPECT_GE(rvec->capacity(), 24u);

reader->LoadEntry(9);
EXPECT_EQ(rvec->size(), 0u);
EXPECT_GE(rvec->capacity(), 24u);

reader->LoadEntry(10);
EXPECT_EQ(rvec->size(), 100u);
EXPECT_GE(rvec->capacity(), 100u);

reader->LoadEntry(11);
EXPECT_EQ(rvec->size(), 101u);
EXPECT_GE(rvec->capacity(), 150u);

reader->LoadEntry(12);
EXPECT_EQ(rvec->size(), 2u);
EXPECT_GE(rvec->capacity(), 75u);

reader->LoadEntry(13);
EXPECT_EQ(rvec->size(), 3u);
EXPECT_GE(rvec->capacity(), 37u);
}

TEST(RNTuple, BoolVector)
{
FileRaii fileGuard("test_ntuple_boolvec.root");
Expand Down Expand Up @@ -352,6 +452,8 @@ void FillComplexVector(const std::string &fname)
auto model = RNTupleModel::Create();
auto wrV = model->MakeField<Coll_t<ComplexStruct>>("v");

// Sequence of number of elements: 0, 10, 10000, 100, 200, 0, 1

auto ntuple = RNTupleWriter::Recreate(std::move(model), "T", fname);
ntuple->Fill();
for (unsigned i = 0; i < 10; ++i)
Expand All @@ -377,6 +479,7 @@ void FillComplexVector(const std::string &fname)
template <template <typename> class Coll_t>
void TestComplexVector(const std::string &fname)
{
static constexpr bool canShrink = std::is_base_of_v<ROOT::Internal::VecOps::SmallVectorBase, Coll_t<ComplexStruct>>;
ComplexStruct::SetNCallConstructor(0);
ComplexStruct::SetNCallDestructor(0);
{
Expand All @@ -396,23 +499,23 @@ void TestComplexVector(const std::string &fname)
EXPECT_EQ(10, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(10000u, rdV->size());
ntuple->LoadEntry(3);
EXPECT_EQ(10010, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(9910, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(canShrink ? 10110 : 10010, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(canShrink ? 10010 : 9910, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(100u, rdV->size());
ntuple->LoadEntry(4);
EXPECT_EQ(10110, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(9910, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(canShrink ? 10310 : 10110, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(canShrink ? 10110 : 9910, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(200u, rdV->size());
ntuple->LoadEntry(5);
EXPECT_EQ(10110, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(10110, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(canShrink ? 10310 : 10110, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(canShrink ? 10310 : 10110, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(0u, rdV->size());
ntuple->LoadEntry(6);
EXPECT_EQ(10111, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(10110, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(canShrink ? 10311 : 10111, ComplexStruct::GetNCallConstructor());
EXPECT_EQ(canShrink ? 10310 : 10110, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(1u, rdV->size());
}
EXPECT_EQ(10111u, ComplexStruct::GetNCallDestructor());
EXPECT_EQ(canShrink ? 10311 : 10111u, ComplexStruct::GetNCallDestructor());
}

TEST(RNTuple, ComplexVector)
Expand Down
Loading