[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
2 times, most recently
from
27ddc85 to
ba94832
Compare
Test Results 22 files 22 suites 3d 12h 10m 57s ⏱️ For more details on these failures, see this check. Results for commit e11c471. ♻️ 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
jblomer
force-pushed
the
ntuple-update-align
branch
from
ba94832 to
e11c471
Compare
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