Skip to content

[ntuple] Add alignment calculation in RClassField::Attach method - #17854

Closed
Aditya-138-12 wants to merge 1 commit into
root-project:masterfrom
Aditya-138-12:issue_16765
Closed

[ntuple] Add alignment calculation in RClassField::Attach method#17854
Aditya-138-12 wants to merge 1 commit into
root-project:masterfrom
Aditya-138-12:issue_16765

Conversation

@Aditya-138-12

@Aditya-138-12 Aditya-138-12 commented Mar 1, 2025

Copy link
Copy Markdown
Contributor

This Pull request:

Add alignment calculation in RClassField::Attach method

Changes or fixes:

  1. Get the TypeName of the class
  2. Loop over all the data members, and find maximum alignment requirement
  3. Update the maximum alignment considering both the class members and child fields
  4. This takes into account the transient members along with the persistent members.

Now this

#include <ROOT/RField.hxx>

class ClassWithTransient {
  char a;
  int t; //!
  char b;
};

void ntuple_class_align() {
  ROOT::Experimental::RField<ClassWithTransient> f("f");
  std::cout << "value size: " << f.GetValueSize() << ", alignment: " << f.GetAlignment() << "\n";
}

Return this
value size: 12, alignment: 4

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #16765

1. Get the TypeName of the class
2. Loop over all the data members, and find maximum alignment requirement
3. Update the maximum alignment considering both the class members and child fields
4. This takes into account the transient members along with the persistent members.
@Aditya-138-12
Aditya-138-12 requested a review from jblomer as a code owner March 1, 2025 18:34
@hahnjo hahnjo self-assigned this Mar 2, 2025
@dpiparo
dpiparo requested review from hahnjo and silverweed March 2, 2025 21:24

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

Hi @Aditya-138-12, thanks for the PR! Some thoughts:

  1. RClassField::Attach is called for every (persistent) member, which means the added code is executed multiple times. The result is probably still correct, but this can get expensive for very big classes with many members.
  2. Using TDataMember::GetUnitSize() returns the size of the member, not its alignment. That works ok for fundamental types, but if the member is itself an object, it will lead to severe overalignment.

I think the proper way to fix this problem is exposing alignment information from ROOT Meta. And once we have that, I think we should not try to determine the alignment ourselves but call a (to-be-implemented) TClass::GetAlignment directly.

@Aditya-138-12

Aditya-138-12 commented Mar 4, 2025

Copy link
Copy Markdown
Contributor Author

Hi @hahnjo,
I was thinking for the complex classes, maybe we can try recursive approach but it would be expensive too...
I will try to explore ROOT's Meta for this... as the TClass::GetAlignment is not yet implemented, can I give it a try or will it be taken by experts?
Thank you so much for your time!

@Aditya-138-12
Aditya-138-12 requested a review from hahnjo March 5, 2025 08:55
@hahnjo

hahnjo commented Mar 5, 2025

Copy link
Copy Markdown
Member

I was thinking for the complex classes, maybe we can try recursive approach but it would be expensive too...

In addition it might not even be possible via the RField API because there might be transient members without RNTuple I/O support.

I will try to explore ROOT's Meta for this... as the TClass::GetAlignment is not yet implemented, can I give it a try or will it be taken by experts?

You can give it a try, just be aware that ROOT Meta and TClass are quite complex. You might also need more info from Cling and / or Clang, which are even more complex...

@Aditya-138-12

Aditya-138-12 commented Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

Hi @hahnjo,
I gave it a try but I was overwhelmed by the complexity of the code base, maybe I should leave this to the relevant ROOT experts.
Thank you for your time!!

@Aditya-138-12
Aditya-138-12 deleted the issue_16765 branch March 20, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ntuple] Wrong alignment with transient class member

2 participants