Skip to content

Commit e84b44c

Browse files
jblomerjblomer
committed
[ntuple] use TClass alignment info where possible
Fixes RNTuple alignment of classes whose alignment is determined by a transient member.
1 parent 17b4518 commit e84b44c

13 files changed

Lines changed: 145 additions & 36 deletions

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,6 @@ private:
167167
TClass *fClass;
168168
/// Additional information kept for each entry in `fSubfields`
169169
std::vector<RSubfieldInfo> fSubfieldsInfo;
170-
std::size_t fMaxAlignment = 1;
171170

172171
/// The staging area stores inputs to I/O rules according to the offsets given by the streamer info of
173172
/// "TypeName@@Version". The area is allocated depending on I/O rules resp. the source members of the I/O rules.
@@ -222,8 +221,8 @@ public:
222221
~RClassField() override;
223222

224223
std::vector<RValue> SplitValue(const RValue &value) const final;
225-
size_t GetValueSize() const final;
226-
size_t GetAlignment() const final { return fMaxAlignment; }
224+
std::size_t GetValueSize() const final;
225+
std::size_t GetAlignment() const final;
227226
std::uint32_t GetTypeVersion() const final;
228227
std::uint32_t GetTypeChecksum() const final;
229228
/// For polymorphic classes (that declare or inherit at least one virtual method), return the expected dynamic type

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,8 +176,8 @@ public:
176176
~RProxiedCollectionField() override = default;
177177

178178
std::vector<RValue> SplitValue(const RValue &value) const final;
179-
size_t GetValueSize() const final { return fProxy->Sizeof(); }
180-
size_t GetAlignment() const final { return alignof(std::max_align_t); }
179+
std::size_t GetValueSize() const final;
180+
std::size_t GetAlignment() const final;
181181
void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final;
182182
};
183183

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

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -264,8 +264,8 @@ public:
264264
~ROptionalField() override = default;
265265

266266
std::vector<RValue> SplitValue(const RValue &value) const final;
267-
size_t GetValueSize() const final;
268-
size_t GetAlignment() const final;
267+
std::size_t GetValueSize() const final;
268+
std::size_t GetAlignment() const final;
269269
};
270270

271271
template <typename ItemT>
@@ -313,8 +313,8 @@ public:
313313
~RUniquePtrField() override = default;
314314

315315
std::vector<RValue> SplitValue(const RValue &value) const final;
316-
size_t GetValueSize() const final { return sizeof(std::unique_ptr<char>); }
317-
size_t GetAlignment() const final { return alignof(std::unique_ptr<char>); }
316+
std::size_t GetValueSize() const final { return sizeof(std::unique_ptr<char>); }
317+
std::size_t GetAlignment() const final { return alignof(std::unique_ptr<char>); }
318318
};
319319

320320
template <typename ItemT>
@@ -363,8 +363,8 @@ public:
363363
RField &operator=(RField &&other) = default;
364364
~RField() final = default;
365365

366-
size_t GetValueSize() const final { return sizeof(std::string); }
367-
size_t GetAlignment() const final { return std::alignment_of<std::string>(); }
366+
std::size_t GetValueSize() const final { return sizeof(std::string); }
367+
std::size_t GetAlignment() const final { return alignof(std::string); }
368368
void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final;
369369
};
370370

@@ -436,8 +436,8 @@ public:
436436
RVariantField &operator=(RVariantField &&other) = default;
437437
~RVariantField() override = default;
438438

439-
size_t GetValueSize() const final;
440-
size_t GetAlignment() const final;
439+
std::size_t GetValueSize() const final;
440+
std::size_t GetAlignment() const final;
441441
};
442442

443443
template <typename... ItemTs>

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -261,8 +261,8 @@ public:
261261
CreateUntyped(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField);
262262

263263
std::vector<RValue> SplitValue(const RValue &value) const final;
264-
size_t GetValueSize() const final { return sizeof(std::vector<char>); }
265-
size_t GetAlignment() const final { return std::alignment_of<std::vector<char>>(); }
264+
std::size_t GetValueSize() const final;
265+
std::size_t GetAlignment() const final;
266266
void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final;
267267
};
268268

@@ -315,8 +315,8 @@ public:
315315

316316
std::vector<RValue> SplitValue(const RValue &value) const final;
317317

318-
size_t GetValueSize() const final { return sizeof(std::vector<bool>); }
319-
size_t GetAlignment() const final { return std::alignment_of<std::vector<bool>>(); }
318+
std::size_t GetValueSize() const final { return sizeof(std::vector<bool>); }
319+
std::size_t GetAlignment() const final { return alignof(std::vector<bool>); }
320320
void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final;
321321
};
322322

@@ -410,8 +410,8 @@ public:
410410
RArrayAsVectorField &operator=(RArrayAsVectorField &&other) = default;
411411
~RArrayAsVectorField() final = default;
412412

413-
size_t GetValueSize() const final { return sizeof(std::vector<char>); }
414-
size_t GetAlignment() const final { return std::alignment_of<std::vector<char>>(); }
413+
std::size_t GetValueSize() const final;
414+
std::size_t GetAlignment() const final;
415415

416416
std::vector<RFieldBase::RValue> SplitValue(const RFieldBase::RValue &value) const final;
417417
void AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const final;

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@ class RSoAField : public RFieldBase {
6666
std::vector<std::size_t> fSoAMemberOffsets; ///< The offset of the RVec members in the SoA type
6767
std::vector<std::size_t> fRecordMemberIndexes; ///< Maps the SoA members to the members of the underlying record
6868
std::vector<RFieldBase *> fRecordMemberFields; ///< Direct access to the member fields of the underlying record
69-
std::size_t fMaxAlignment = 1;
7069
ROOT::Internal::RColumnIndex fNWritten;
7170

7271
RSoAField(std::string_view fieldName, const RSoAField &source); ///< Used by CloneImpl
@@ -96,8 +95,8 @@ public:
9695
~RSoAField() override = default;
9796

9897
std::vector<RValue> SplitValue(const RValue &value) const final;
99-
size_t GetValueSize() const final;
100-
size_t GetAlignment() const final { return fMaxAlignment; }
98+
std::size_t GetValueSize() const final;
99+
std::size_t GetAlignment() const final;
101100
/// For polymorphic classes (that declare or inherit at least one virtual method), return the expected dynamic type
102101
/// of any user object. If the class is not polymorphic, return nullptr.
103102
/// TODO(jblomer): use information in unique pointer field

‎tree/ntuple/inc/ROOT/RFieldBase.hxx‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,9 @@ protected:
552552
const ROOT::RNTupleDescriptor *desc, ROOT::DescriptorId_t fieldId);
553553

554554
public:
555+
/// Maximum supported alignment for field types, i.e. maximum returned by GetAlignment()
556+
static constexpr std::size_t kMaxAlignment = 4096;
557+
555558
template <bool IsConstT>
556559
class RSchemaIteratorTemplate;
557560
using RSchemaIterator = RSchemaIteratorTemplate<false>;
@@ -620,10 +623,9 @@ public:
620623
/// correct `std::variant` or all the elements of a collection. The default implementation assumes no subvalues
621624
/// and returns an empty vector.
622625
virtual std::vector<RValue> SplitValue(const RValue &value) const;
623-
/// The number of bytes taken by a value of the appropriate type
626+
/// What sizeof(T) for this type returns
624627
virtual size_t GetValueSize() const = 0;
625-
/// As a rule of thumb, the alignment is equal to the size of the type. There are, however, various exceptions
626-
/// to this rule depending on OS and CPU architecture. So enforce the alignment to be explicitly spelled out.
628+
/// What alignof(T) for this type returns
627629
virtual size_t GetAlignment() const = 0;
628630
std::uint32_t GetTraits() const { return fTraits; }
629631
bool HasReadCallbacks() const { return !fReadCallbacks.empty(); }

‎tree/ntuple/inc/ROOT/RFieldUtils.hxx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,9 @@ std::size_t EvalRVecValueSize(std::size_t alignOfT, std::size_t sizeOfT, std::si
110110
std::size_t EvalRVecAlignment(std::size_t alignOfSubfield);
111111
void DestroyRVecWithChecks(std::size_t alignOfT, unsigned char **beginPtr, std::int32_t *capacityPtr);
112112

113+
// TODO: move to upcoming BitUtils.hxx
114+
inline bool IsPowerOfTwo(std::uint64_t v) { return (v & (v - 1)) == 0; }
115+
113116
} // namespace Internal
114117
} // namespace ROOT
115118

‎tree/ntuple/src/RFieldMeta.cxx‎

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,12 @@ TEnum *EnsureValidEnum(std::string_view enumName)
117117
return e;
118118
}
119119

120+
void EnsureValidAlignment(std::size_t align)
121+
{
122+
if (align == 0 || align > ROOT::RFieldBase::kMaxAlignment || !ROOT::Internal::IsPowerOfTwo(align))
123+
throw ROOT::RException(R__FAIL(std::string("invalid alignment: ") + std::to_string(align)));
124+
}
125+
120126
/// Create a comma-separated list of type names from the given fields. Uses either the real type names or the
121127
/// type aliases (if there are any, otherwise the actual type name). Used to construct template argument lists
122128
/// for templated types such as std::pair<...>, std::tuple<...>, std::variant<...>.
@@ -187,8 +193,7 @@ std::string BuildMapTypeName(ROOT::RMapField::EMapType mapType, const ROOT::RFie
187193
ROOT::RClassField::RClassField(std::string_view fieldName, const RClassField &source)
188194
: ROOT::RFieldBase(fieldName, source.GetTypeName(), ROOT::ENTupleStructure::kRecord, false /* isSimple */),
189195
fClass(source.fClass),
190-
fSubfieldsInfo(source.fSubfieldsInfo),
191-
fMaxAlignment(source.fMaxAlignment)
196+
fSubfieldsInfo(source.fSubfieldsInfo)
192197
{
193198
for (const auto &f : source.GetConstSubfields()) {
194199
RFieldBase::Attach(f->Clone(f->GetFieldName()));
@@ -281,7 +286,6 @@ ROOT::RClassField::~RClassField()
281286

282287
void ROOT::RClassField::Attach(std::unique_ptr<RFieldBase> child, RSubfieldInfo info)
283288
{
284-
fMaxAlignment = std::max(fMaxAlignment, child->GetAlignment());
285289
fSubfieldsInfo.push_back(info);
286290
RFieldBase::Attach(std::move(child));
287291
}
@@ -623,11 +627,18 @@ std::vector<ROOT::RFieldBase::RValue> ROOT::RClassField::SplitValue(const RValue
623627
return result;
624628
}
625629

626-
size_t ROOT::RClassField::GetValueSize() const
630+
std::size_t ROOT::RClassField::GetValueSize() const
627631
{
628632
return fClass->GetClassSize();
629633
}
630634

635+
std::size_t ROOT::RClassField::GetAlignment() const
636+
{
637+
const auto align = fClass->GetClassAlignment();
638+
EnsureValidAlignment(align);
639+
return align;
640+
}
641+
631642
std::uint32_t ROOT::RClassField::GetTypeVersion() const
632643
{
633644
return fClass->GetClassVersion();
@@ -658,8 +669,7 @@ ROOT::Experimental::RSoAField::RSoAField(std::string_view fieldName, const RSoAF
658669
: ROOT::RFieldBase(fieldName, source.GetTypeName(), ROOT::ENTupleStructure::kCollection, false /* isSimple */),
659670
fSoAClass(source.fSoAClass),
660671
fSoAMemberOffsets(source.fSoAMemberOffsets),
661-
fRecordMemberIndexes(source.fRecordMemberIndexes),
662-
fMaxAlignment(source.fMaxAlignment)
672+
fRecordMemberIndexes(source.fRecordMemberIndexes)
663673
{
664674
fTraits = source.GetTraits();
665675
Attach(source.fSubfields[0]->Clone(source.fSubfields[0]->GetFieldName()));
@@ -750,8 +760,6 @@ ROOT::Experimental::RSoAField::RSoAField(std::string_view fieldName, TClass *clS
750760
leftType + " vs. " + rightType + ")"));
751761
}
752762

753-
fMaxAlignment = std::max(fMaxAlignment, vecField->GetAlignment());
754-
755763
fSoAMemberOffsets.emplace_back(dataMember->GetOffset());
756764
fRecordMemberIndexes.emplace_back(itr->second);
757765
}
@@ -856,11 +864,18 @@ std::vector<ROOT::RFieldBase::RValue> ROOT::Experimental::RSoAField::SplitValue(
856864
return std::vector<RValue>();
857865
}
858866

859-
size_t ROOT::Experimental::RSoAField::GetValueSize() const
867+
std::size_t ROOT::Experimental::RSoAField::GetValueSize() const
860868
{
861869
return fSoAClass->GetClassSize();
862870
}
863871

872+
std::size_t ROOT::Experimental::RSoAField::GetAlignment() const
873+
{
874+
const auto align = fSoAClass->GetClassAlignment();
875+
EnsureValidAlignment(align);
876+
return align;
877+
}
878+
864879
const std::type_info *ROOT::Experimental::RSoAField::GetPolymorphicTypeInfo() const
865880
{
866881
// TODO(jblomer): factor out
@@ -1181,6 +1196,18 @@ std::vector<ROOT::RFieldBase::RValue> ROOT::RProxiedCollectionField::SplitValue(
11811196
return result;
11821197
}
11831198

1199+
std::size_t ROOT::RProxiedCollectionField::GetValueSize() const
1200+
{
1201+
return fProxy->Sizeof();
1202+
}
1203+
1204+
std::size_t ROOT::RProxiedCollectionField::GetAlignment() const
1205+
{
1206+
const auto align = fProxy->GetCollectionClass()->GetClassAlignment();
1207+
EnsureValidAlignment(align);
1208+
return align;
1209+
}
1210+
11841211
void ROOT::RProxiedCollectionField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const
11851212
{
11861213
visitor.VisitProxiedCollectionField(*this);
@@ -1388,7 +1415,9 @@ ROOT::RExtraTypeInfoDescriptor ROOT::RStreamerField::GetExtraTypeInfo() const
13881415

13891416
std::size_t ROOT::RStreamerField::GetAlignment() const
13901417
{
1391-
return std::min(alignof(std::max_align_t), GetValueSize()); // TODO(jblomer): fix me
1418+
const auto align = fClass->GetClassAlignment();
1419+
EnsureValidAlignment(align);
1420+
return align;
13921421
}
13931422

13941423
std::size_t ROOT::RStreamerField::GetValueSize() const

‎tree/ntuple/src/RFieldSequenceContainer.cxx‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
/// \author Jonas Hahnfeld <jonas.hahnfeld@cern.ch>
44
/// \date 2024-11-19
55

6+
#include <ROOT/RAlignmentUtils.hxx>
67
#include <ROOT/RField.hxx>
78
#include <ROOT/RFieldBase.hxx>
89
#include <ROOT/RFieldVisitor.hxx>
@@ -30,6 +31,20 @@ std::vector<ROOT::RFieldBase::RValue> SplitVector(std::shared_ptr<void> valuePtr
3031
return result;
3132
}
3233

34+
std::size_t GetSizeOfVector()
35+
{
36+
static_assert(sizeof(std::vector<char>) ==
37+
sizeof(std::vector<ROOT::Internal::AlignedStorage<ROOT::RFieldBase::kMaxAlignment>>));
38+
return sizeof(std::vector<char>);
39+
}
40+
41+
std::size_t GetAlignOfVector()
42+
{
43+
static_assert(alignof(std::vector<char>) ==
44+
alignof(std::vector<ROOT::Internal::AlignedStorage<ROOT::RFieldBase::kMaxAlignment>>));
45+
return alignof(std::vector<char>);
46+
}
47+
3348
} // anonymous namespace
3449

3550
ROOT::RArrayField::RArrayField(std::string_view fieldName, std::unique_ptr<RFieldBase> itemField,
@@ -648,6 +663,16 @@ std::vector<ROOT::RFieldBase::RValue> ROOT::RVectorField::SplitValue(const RValu
648663
return SplitVector(value.GetPtr<void>(), *fSubfields[0]);
649664
}
650665

666+
std::size_t ROOT::RVectorField::GetValueSize() const
667+
{
668+
return GetSizeOfVector();
669+
}
670+
671+
std::size_t ROOT::RVectorField::GetAlignment() const
672+
{
673+
return GetAlignOfVector();
674+
}
675+
651676
void ROOT::RVectorField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const
652677
{
653678
visitor.VisitVectorField(*this);
@@ -975,6 +1000,16 @@ std::vector<ROOT::RFieldBase::RValue> ROOT::RArrayAsVectorField::SplitValue(cons
9751000
return SplitVector(value.GetPtr<void>(), *fSubfields[0]);
9761001
}
9771002

1003+
std::size_t ROOT::RArrayAsVectorField::GetValueSize() const
1004+
{
1005+
return GetSizeOfVector();
1006+
}
1007+
1008+
std::size_t ROOT::RArrayAsVectorField::GetAlignment() const
1009+
{
1010+
return GetAlignOfVector();
1011+
}
1012+
9781013
void ROOT::RArrayAsVectorField::AcceptVisitor(ROOT::Detail::RFieldVisitor &visitor) const
9791014
{
9801015
visitor.VisitArrayAsVectorField(*this);

‎tree/ntuple/test/CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ ROOT_STANDARD_LIBRARY_PACKAGE(CustomStruct
1111
HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/CustomStruct.hxx
1212
SOURCES CustomStruct.cxx
1313
LINKDEF CustomStructLinkDef.h
14-
DEPENDENCIES RIO)
14+
DEPENDENCIES RIO ROOTVecOps)
1515
configure_file(CustomStruct.hxx . COPYONLY)
1616
if(MSVC AND NOT CMAKE_GENERATOR MATCHES Ninja)
1717
add_custom_command(TARGET CustomStruct POST_BUILD

0 commit comments

Comments
 (0)