Record alignment of class in dictionary and use in TStreamerInfo#21669
Record alignment of class in dictionary and use in TStreamerInfo#21669pcanal wants to merge 12 commits intoroot-project:masterfrom
Conversation
Test Results 21 files 21 suites 3d 3h 1m 42s ⏱️ For more details on these failures, see this check. Results for commit dbf6175. ♻️ This comment has been updated with latest results. |
jblomer
left a comment
There was a problem hiding this comment.
Thanks!
I think it would be nice to have a separate test for AlignOf just for the cling part that exercises the different special cases (forward declaration etc.)
| // FIX-ALIGN: do we need to have something special for collection proxy? | ||
| //if (fCollectionProxy) | ||
| // return fCollectionProxy->AlignOf(); |
There was a problem hiding this comment.
I think it would be good to resolve the question in this PR.
core/meta/src/TClass.cxx
Outdated
| // return fCollectionProxy->AlignOf(); | ||
| if (HasInterpreterInfo()) | ||
| return gCling->ClassInfo_AlignOf(GetClassInfo()); | ||
| return GetStreamerInfo()->AlignOf(); |
There was a problem hiding this comment.
This is a good question ... Currently (see updated doc) TClingClassInfo return -1 for error case but 0 for semi-error cases (eg. a namespace). It is unclear whether we want to treat those as an exception (i.e. the caller would get an assert if this is not called on a Class.
| /// Allocator that allocates memory aligned to at least \a Align bytes. | ||
| /// The alignment is chosen at construction time and must be a power of two. | ||
| template <typename T> | ||
| struct AlignedAllocator { |
There was a problem hiding this comment.
This may be useful for RNTuple, too. Perhaps we can move it as a utility class to Core.
There was a problem hiding this comment.
The custom allocator has been removed because it leads to the in-memory layout to be different from regular vector.
There was a problem hiding this comment.
Actually it is still there but not used. Should I recommend or do you recommend I move it?
Since align is a power of 2 the modulo may be written as (offset & (align - 1)) (assuming offset is non-negative) Co-authored-by: silverweed <giacomo.parolini@cern.ch>
This fixes #21667 and thus addresses CMS issue seen at github.com/cms-sw/cmssw/pull/49969
With the current version storage of collection whose content needs to be aligned with more than 4096 is not supported. Extending it to higher number is trivial but require specific handling for each value so we had to stop at an arbitrary value. The error is seen at dictionary compilation time: