Skip to content

Commit aa8c36c

Browse files
jblomerjblomer
committed
[ntuple] fix up vector new/delete for over-aligned values
1 parent 9994ce9 commit aa8c36c

3 files changed

Lines changed: 86 additions & 18 deletions

File tree

‎tree/ntuple/inc/ROOT/RField/RFieldSequenceContainer.hxx‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -219,11 +219,12 @@ class RVectorField : public RFieldBase {
219219
class RVectorDeleter : public RDeleter {
220220
private:
221221
std::size_t fItemSize = 0;
222+
std::size_t fItemAlignment = 0;
222223
std::unique_ptr<RDeleter> fItemDeleter;
223224

224225
public:
225-
RVectorDeleter();
226-
RVectorDeleter(std::size_t itemSize, std::unique_ptr<RDeleter> itemDeleter);
226+
explicit RVectorDeleter(std::size_t itemAlignment);
227+
RVectorDeleter(std::size_t itemSize, std::size_t itemAlignment, std::unique_ptr<RDeleter> itemDeleter);
227228
void operator()(void *objPtr, bool dtorOnly) final;
228229
};
229230

@@ -248,7 +249,7 @@ protected:
248249
void GenerateColumns() final;
249250
void GenerateColumns(const ROOT::RNTupleDescriptor &desc) final;
250251

251-
void ConstructValue(void *where) const final { new (where) std::vector<char>(); }
252+
void ConstructValue(void *where) const final;
252253
std::unique_ptr<RDeleter> GetDeleter() const final;
253254

254255
std::size_t AppendImpl(const void *from) final;
@@ -402,7 +403,7 @@ protected:
402403
void GenerateColumns() final;
403404
using RFieldBase::GenerateColumns;
404405

405-
void ConstructValue(void *where) const final { new (where) std::vector<char>(); }
406+
void ConstructValue(void *where) const final;
406407
/// Returns an RVectorField::RVectorDeleter
407408
std::unique_ptr<RDeleter> GetDeleter() const final;
408409

‎tree/ntuple/src/RFieldSequenceContainer.cxx‎

Lines changed: 73 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,29 @@ std::size_t GetAlignOfVector()
4040
return alignof(std::vector<char>);
4141
}
4242

43+
void ConstructVector(void *where, std::size_t alignOfValue)
44+
{
45+
if (alignOfValue <= sizeof(std::max_align_t)) {
46+
new (where) std::vector<char>();
47+
} else {
48+
static_assert(sizeof(std::max_align_t) >= 8 && ROOT::RVectorField::kMaxItemAlignment == 4096);
49+
// clang-format off
50+
switch (alignOfValue) {
51+
case 16: new (where) std::vector<ROOT::Internal::RAlignedStorage< 16>>(); break;
52+
case 32: new (where) std::vector<ROOT::Internal::RAlignedStorage< 32>>(); break;
53+
case 64: new (where) std::vector<ROOT::Internal::RAlignedStorage< 64>>(); break;
54+
case 128: new (where) std::vector<ROOT::Internal::RAlignedStorage< 128>>(); break;
55+
case 256: new (where) std::vector<ROOT::Internal::RAlignedStorage< 256>>(); break;
56+
case 512: new (where) std::vector<ROOT::Internal::RAlignedStorage< 512>>(); break;
57+
case 1024: new (where) std::vector<ROOT::Internal::RAlignedStorage<1024>>(); break;
58+
case 2048: new (where) std::vector<ROOT::Internal::RAlignedStorage<2048>>(); break;
59+
case 4096: new (where) std::vector<ROOT::Internal::RAlignedStorage<4096>>(); break;
60+
default: throw ROOT::RException(R__FAIL(std::string("Unsupported alignment: ") + std::to_string(alignOfValue)));
61+
}
62+
// clang-format on
63+
}
64+
}
65+
4366
} // anonymous namespace
4467

4568
ROOT::RArrayField::RArrayField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField,
@@ -669,33 +692,65 @@ void ROOT::RVectorField::ReconcileOnDiskField(const RNTupleDescriptor &desc)
669692
EnsureMatchingOnDiskCollection(desc).ThrowOnError();
670693
}
671694

672-
ROOT::RVectorField::RVectorDeleter::RVectorDeleter() : RDeleter(GetAlignOfVector()) {}
695+
void ROOT::RVectorField::ConstructValue(void *where) const
696+
{
697+
ConstructVector(where, fSubfields[0]->GetAlignment());
698+
}
673699

674-
ROOT::RVectorField::RVectorDeleter::RVectorDeleter(std::size_t itemSize, std::unique_ptr<RDeleter> itemDeleter)
675-
: RDeleter(GetAlignOfVector()), fItemSize(itemSize), fItemDeleter(std::move(itemDeleter))
700+
ROOT::RVectorField::RVectorDeleter::RVectorDeleter(std::size_t itemAlignment)
701+
: RDeleter(GetAlignOfVector()), fItemAlignment(itemAlignment)
702+
{
703+
}
704+
705+
ROOT::RVectorField::RVectorDeleter::RVectorDeleter(std::size_t itemSize, std::size_t itemAlignment,
706+
std::unique_ptr<RDeleter> itemDeleter)
707+
: RDeleter(GetAlignOfVector()),
708+
fItemSize(itemSize),
709+
fItemAlignment(itemAlignment),
710+
fItemDeleter(std::move(itemDeleter))
676711
{
677712
}
678713

679714
void ROOT::RVectorField::RVectorDeleter::operator()(void *objPtr, bool dtorOnly)
680715
{
681716
auto vecPtr = static_cast<std::vector<char> *>(objPtr);
682717
if (fItemDeleter) {
683-
R__ASSERT(fItemSize > 0);
684-
R__ASSERT((vecPtr->size() % fItemSize) == 0);
718+
assert(fItemSize > 0);
685719
auto nItems = vecPtr->size() / fItemSize;
720+
assert((vecPtr->size() % fItemSize) == 0);
686721
for (std::size_t i = 0; i < nItems; ++i) {
687722
fItemDeleter->operator()(vecPtr->data() + (i * fItemSize), true /* dtorOnly */);
688723
}
689724
}
690-
std::destroy_at(vecPtr);
725+
726+
if (fItemAlignment <= sizeof(std::max_align_t)) {
727+
std::destroy_at(vecPtr);
728+
} else {
729+
static_assert(sizeof(std::max_align_t) >= 8 && kMaxItemAlignment == 4096);
730+
// clang-format off
731+
switch (fItemAlignment) {
732+
case 16: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage< 16>> *>(objPtr)); break;
733+
case 32: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage< 32>> *>(objPtr)); break;
734+
case 64: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage< 64>> *>(objPtr)); break;
735+
case 128: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage< 128>> *>(objPtr)); break;
736+
case 256: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage< 256>> *>(objPtr)); break;
737+
case 512: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage< 512>> *>(objPtr)); break;
738+
case 1024: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage<1024>> *>(objPtr)); break;
739+
case 2048: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage<2048>> *>(objPtr)); break;
740+
case 4096: std::destroy_at(static_cast<std::vector<Internal::RAlignedStorage<4096>> *>(objPtr)); break;
741+
default: R__ASSERT(false);
742+
}
743+
// clang-format on
744+
}
745+
691746
RDeleter::operator()(objPtr, dtorOnly);
692747
}
693748

694749
std::unique_ptr<ROOT::RFieldBase::RDeleter> ROOT::RVectorField::GetDeleter() const
695750
{
696751
if (fItemDeleter)
697-
return std::make_unique<RVectorDeleter>(fItemSize, GetDeleterOf(*fSubfields[0]));
698-
return std::make_unique<RVectorDeleter>();
752+
return std::make_unique<RVectorDeleter>(fItemSize, fSubfields[0]->GetAlignment(), GetDeleterOf(*fSubfields[0]));
753+
return std::make_unique<RVectorDeleter>(fSubfields[0]->GetAlignment());
699754
}
700755

701756
std::vector<ROOT::RFieldBase::RValue> ROOT::RVectorField::SplitValue(const RValue &value) const
@@ -982,11 +1037,18 @@ void ROOT::RArrayAsVectorField::GenerateColumns()
9821037
throw RException(R__FAIL("RArrayAsVectorField fields must only be used for reading"));
9831038
}
9841039

1040+
void ROOT::RArrayAsVectorField::ConstructValue(void *where) const
1041+
{
1042+
ConstructVector(where, fSubfields[0]->GetAlignment());
1043+
}
1044+
9851045
std::unique_ptr<ROOT::RFieldBase::RDeleter> ROOT::RArrayAsVectorField::GetDeleter() const
9861046
{
987-
if (fItemDeleter)
988-
return std::make_unique<RVectorField::RVectorDeleter>(fItemSize, GetDeleterOf(*fSubfields[0]));
989-
return std::make_unique<RVectorField::RVectorDeleter>();
1047+
if (fItemDeleter) {
1048+
return std::make_unique<RVectorField::RVectorDeleter>(fItemSize, fSubfields[0]->GetAlignment(),
1049+
GetDeleterOf(*fSubfields[0]));
1050+
}
1051+
return std::make_unique<RVectorField::RVectorDeleter>(fSubfields[0]->GetAlignment());
9901052
}
9911053

9921054
void ROOT::RArrayAsVectorField::ReadGlobalImpl(ROOT::NTupleSize_t globalIndex, void *to)

‎tree/ntuple/test/rfield_class.cxx‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -518,13 +518,18 @@ TEST(RNTuple, AlignmentCornerCases)
518518
auto f = RFieldBase::Create("", "AlignmentEnvelope").Unwrap();
519519
EXPECT_GT(alignof(AlignmentEnvelope), sizeof(std::max_align_t));
520520
EXPECT_EQ(alignof(AlignmentEnvelope), f->GetAlignment());
521+
std::unique_ptr<AlignmentEnvelope> ptr(f->CreateObject<AlignmentEnvelope>().release());
522+
EXPECT_EQ(0, reinterpret_cast<std::uintptr_t>(ptr.get()) % alignof(AlignmentEnvelope));
523+
524+
f = RFieldBase::Create("", "std::vector<AlignmentEnvelope>").Unwrap();
525+
auto vecPtr = f->CreateObject<std::vector<AlignmentEnvelope>>().release();
526+
EXPECT_TRUE(vecPtr->data() == nullptr ||
527+
reinterpret_cast<std::uintptr_t>(vecPtr->data()) % alignof(AlignmentEnvelope) == 0);
528+
521529
auto res = RFieldBase::Create("", "ROOT::RVec<OverAligned>");
522530
EXPECT_FALSE(res);
523531
EXPECT_THAT(res.GetError()->GetReport(), ::testing::HasSubstr("RVec does not support over-aligned types"));
524532

525-
std::unique_ptr<AlignmentEnvelope> ptr(f->CreateObject<AlignmentEnvelope>().release());
526-
EXPECT_EQ(0, reinterpret_cast<std::uintptr_t>(ptr.get()) % alignof(AlignmentEnvelope));
527-
528533
FileRaii fileGuard("test_ntuple_alignment_corner_cases.root");
529534
{
530535
auto model = ROOT::RNTupleModel::Create();

0 commit comments

Comments
 (0)