diff --git a/tree/ntuple/src/RFieldSequenceContainer.cxx b/tree/ntuple/src/RFieldSequenceContainer.cxx index 7505249e14b86..bea2179cae7dc 100644 --- a/tree/ntuple/src/RFieldSequenceContainer.cxx +++ b/tree/ntuple/src/RFieldSequenceContainer.cxx @@ -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 */ { // 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>) 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) { 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::numeric_limits::max()); + const std::size_t oldCap = static_cast(*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 is over-aligned. - *beginPtr = static_cast(malloc(nItems * itemSize)); + *beginPtr = static_cast(malloc(newCapacity * itemSize)); R__ASSERT(*beginPtr != nullptr); - *capacityPtr = nItems; + *capacityPtr = static_cast(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)); } } diff --git a/tree/ntuple/test/rfield_vector.cxx b/tree/ntuple/test/rfield_vector.cxx index af28671b25961..53da854fef285 100644 --- a/tree/ntuple/test/rfield_vector.cxx +++ b/tree/ntuple/test/rfield_vector.cxx @@ -154,7 +154,6 @@ TEST(RNTuple, InsideCollection) // TODO: test reading of "klassVec.v1" } - TEST(RNTuple, RVec) { FileRaii fileGuard("test_ntuple_rvec.root"); @@ -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>("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>("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"); @@ -352,6 +452,8 @@ void FillComplexVector(const std::string &fname) auto model = RNTupleModel::Create(); auto wrV = model->MakeField>("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) @@ -377,6 +479,7 @@ void FillComplexVector(const std::string &fname) template