[ntuple] Fixes for alignment#22312
Conversation
jblomer
force-pushed
the
ntuple-update-align
branch
from
bb61c3d to
694bd02
Compare
jblomer
force-pushed
the
ntuple-update-align
branch
from
694bd02 to
27ddc85
Compare
Fixes RNTuple alignment of classes whose alignment is determined by a transient member.
jblomer
force-pushed
the
ntuple-update-align
branch
from
27ddc85 to
ba94832
Compare
Test Results 16 files 16 suites 2d 8h 34m 9s ⏱️ Results for commit ba94832. ♻️ This comment has been updated with latest results. |
| return (value + align - 1) & ~(align - 1); | ||
| } | ||
|
|
||
| /// Storage type whose alignment matches \a Align bytes. |
There was a problem hiding this comment.
| /// Storage type whose alignment matches \a Align bytes. | |
| /// Storage type whose alignment matches \a AlignT bytes. |
| #endif // _MSC_VER | ||
| } | ||
|
|
||
| inline bool IsPowerOfTwo(std::uint64_t v) |
There was a problem hiding this comment.
You may (or may not) want to use this in IsValidAlignment (which is only different as it reject 0)
|
|
||
| void ROOT::RFieldBase::EnsureValidAlignment(std::size_t alignment) | ||
| { | ||
| if (alignment == 0 || alignment > ROOT::RFieldBase::kMaxAlignment || !ROOT::Internal::IsPowerOfTwo(alignment)) |
There was a problem hiding this comment.
This is similar (but different) to ROOT::Internal::IsValidAlignment.
| if (alignment == 0 || alignment > ROOT::RFieldBase::kMaxAlignment || !ROOT::Internal::IsPowerOfTwo(alignment)) | |
| if (!ROOT::Internal::IsValidAlignment(alignment) || alignment > ROOT::RFieldBase::kMaxAlignment) |
At least in the main I/O part the limitation to 4096 is only for std::vector because they can be handled without dictionary (but then we need to explicitly handle the alignment of their content.
Is RNTuple more restrictive (i.e. calling an generically provided call to new for various alignment?)
There was a problem hiding this comment.
Actually it isn't. Assuming that the std::vector layout doesn't change depending on the alignment of its item type, RNTuple has no further maximum alignment restrictions of its own. At the moment, I use the maximum alignment to verify that the std::vector alignment is independent.
In general, I thought that it makes sense to have either no size limit or one size limit for all types.
hahnjo
left a comment
hahnjo
left a comment
There was a problem hiding this comment.
The code changes look good overall; quickly looking at the failures, it appears that macOS and Windows are complaining about the missing import of <array>. Not sure if that solves all issues...
|
|
||
| struct AlignmentEnvelope { | ||
| ROOT::RVec<OverAligned> fVec; | ||
| std::array<OverAligned, 2> fArr; |
There was a problem hiding this comment.
As far as I understand, both macOS and Windows are complaining about the missing import of <array>...
hahnjo
left a comment
hahnjo
left a comment
There was a problem hiding this comment.
As discussed offline, this is missing the part of aligning the dynamic array in std::vector<T> (relatively easy to do) and RVec<T> (harder because it currently uses malloc and free). In the worst case, we discussed throwing in case we have alignment larger than std::max_align_t
4 participants
Use the new
TClassalignment information where applicable. This fixesAdditionally, use the
newanddeleteoperators with alignment info. This ensures correct memory allocation for over-aligned types.Fixes #16765