Skip to content

[ntuple] Fixes for alignment#22312

Merged
jblomer merged 7 commits into
root-project:masterfrom
jblomer:ntuple-update-align
Jun 11, 2026
Merged

[ntuple] Fixes for alignment#22312
jblomer merged 7 commits into
root-project:masterfrom
jblomer:ntuple-update-align

Conversation

@jblomer

@jblomer jblomer commented May 16, 2026

Copy link
Copy Markdown
Contributor

Use the new TClass alignment information where applicable. This fixes

  • Alignment of classes whose alignment is defined by a transient member
  • Alignment of certain streamer fields

Additionally, use the new and delete operators with alignment info. This ensures correct memory allocation for over-aligned types.

Fixes #16765

@jblomer jblomer self-assigned this May 16, 2026
@jblomer jblomer requested a review from bellenot as a code owner May 16, 2026 05:32
@jblomer jblomer requested review from dpiparo and pcanal as code owners May 16, 2026 05:32
@jblomer jblomer force-pushed the ntuple-update-align branch from bb61c3d to 694bd02 Compare May 16, 2026 07:25
@jblomer jblomer requested review from enirolf, hahnjo and silverweed May 16, 2026 07:26
@jblomer jblomer force-pushed the ntuple-update-align branch 2 times, most recently from 27ddc85 to ba94832 Compare May 16, 2026 07:42
@github-actions

github-actions Bot commented May 16, 2026

Copy link
Copy Markdown

Test Results

    22 files      22 suites   3d 14h 33m 49s ⏱️
 3 859 tests  3 856 ✅   0 💤 3 ❌
76 253 runs  76 145 ✅ 105 💤 3 ❌

For more details on these failures, see this check.

Results for commit 39576f7.

♻️ This comment has been updated with latest results.

@jblomer jblomer added the clean build Ask CI to do non-incremental build on PR label May 16, 2026
@jblomer jblomer closed this May 16, 2026
@jblomer jblomer reopened this May 16, 2026
Comment thread core/foundation/inc/ROOT/BitUtils.hxx Outdated
Comment thread core/foundation/inc/ROOT/BitUtils.hxx Outdated
Comment thread tree/ntuple/src/RFieldBase.cxx Outdated

@hahnjo hahnjo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Comment thread tree/ntuple/test/CustomStruct.hxx

@hahnjo hahnjo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jblomer force-pushed the ntuple-update-align branch 3 times, most recently from 787669e to 2c2dc4a Compare May 26, 2026 11:59
@jblomer jblomer requested review from hahnjo and pcanal May 26, 2026 20:14
@jblomer jblomer removed the clean build Ask CI to do non-incremental build on PR label May 26, 2026
@jblomer jblomer closed this May 26, 2026
@jblomer jblomer reopened this May 26, 2026
Comment thread tree/ntuple/src/RFieldBase.cxx Outdated
@jblomer jblomer added the clean build Ask CI to do non-incremental build on PR label May 27, 2026
@jblomer jblomer force-pushed the ntuple-update-align branch from 2c2dc4a to aa8c36c Compare June 1, 2026 13:44
@jblomer jblomer requested a review from pcanal June 1, 2026 13:45
Comment thread tree/ntuple/src/RFieldSequenceContainer.cxx Outdated
Comment thread tree/ntuple/src/RFieldSequenceContainer.cxx Outdated

@pcanal pcanal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@jblomer jblomer force-pushed the ntuple-update-align branch 2 times, most recently from 074d861 to 410b9d9 Compare June 2, 2026 13:27

@hahnjo hahnjo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in principle. Windows is unhappy, which I think is because of aligned operator new in the header / just-in-time-compiled code. Maybe this can be solved by outlining the function?

Comment thread tree/ntuple/inc/ROOT/RFieldBase.hxx Outdated
Comment thread tree/ntuple/src/RFieldSequenceContainer.cxx
Comment thread tree/ntuple/src/RFieldSequenceContainer.cxx Outdated
Comment thread tree/ntuple/src/RFieldSequenceContainer.cxx Outdated
@jblomer jblomer force-pushed the ntuple-update-align branch from 410b9d9 to 2b8e789 Compare June 8, 2026 13:54
@jblomer jblomer force-pushed the ntuple-update-align branch from 2b8e789 to 39576f7 Compare June 8, 2026 15:26
@jblomer jblomer merged commit eaaf401 into root-project:master Jun 11, 2026
55 of 58 checks passed
@jblomer jblomer deleted the ntuple-update-align branch June 11, 2026 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clean build Ask CI to do non-incremental build on PR in:RNTuple Small Bug Density

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ntuple] Wrong alignment with transient class member

4 participants