Introduce macros to use reflex attributes #40435

fwyzard · 2023-01-06T11:05:22Z

PR description:

The way dictionary informations are propagated from the C++ code or XML dictionaries to reflex and cling is rather roundabout:

<field name="data_" comment="!"/> tags in XML dictionaries are parsed by genreflex and injected into the LLVM AST of the corresponding C++ code as comments //!;
C++ comments like //! or //[size_] are converted by genreflex/rootcling into LLVM AST annotations;
cling parses the LLVM annotations and uses them to generate the desired behaviour in the dictionaries.

This approach does not work well with macro-generated data members:

macros cannot generate comments, so //! or //[size_] cannot be used directly;
macros cannot easily be used to generate the class_def.xml file, requiring manual intervention for their implementation and maintenance.

However, it turns out that dictionaries can bypass the comments and use LLVM annotations directly within the C++ code. So

private:
  int size_;
  float* data_;       //[size_]
  float* transient_;  //!

can be also expressed as

private:
  int size_;
  float* data_ [[clang::annotate("[size_]")]];
  float* transient_ [[clang::annotate("!")]];

and annotations can be generated by macros.

A new header file, FWCore/Reflection/interface/reflex.h, currently implements two macros:

EDM_REFLEX_TRANSIENT can be used to annotate transient data members, like //!
EDM_REFLEX_SIZE(SIZE) can be used to annotate dynamic arrays, like //[SIZE]

To avoid warning about unrecognised attributes, these macros expand to nothing unless __CLING__ is defined.

The changes to DataFormats/SoATemplate/interface/SoALayout.h add these macros to the SoA data members, so they do not need to be explicitly added by hand to the various classes_def.xml files.

PR validation:

The SoA unit tests under HeterogeneousCore/AlpakaTest and HeterogeneousCore/CUDATest pass.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

No backport expected.

fwyzard · 2023-01-06T11:05:34Z

please test

fwyzard · 2023-01-06T11:05:45Z

@ericcano FYI

fwyzard · 2023-01-06T11:06:02Z

@makortel @Dr15Jones @pcanal what do you think ?

cmsbuild · 2023-01-06T11:12:13Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40435/33576

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/PortableTestObjects/src/classes_def.xml modified in PR(s): Add support to multi layout portable collections (any number) #40285

cmsbuild · 2023-01-06T11:12:36Z

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

DataFormats/PortableTestObjects (heterogeneous)
DataFormats/SoATemplate (heterogeneous)
FWCore/Utilities (core)

@makortel, @smuzaffar, @Dr15Jones, @fwyzard can you please review it and eventually sign? Thanks.
@makortel, @missirol, @rovere, @wddgit, @felicepantaleo this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2023-01-06T15:01:04Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7fe410/29811/summary.html
COMMIT: 1c5c842
CMSSW: CMSSW_13_0_X_2023-01-06-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40435/29811/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 3 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555748
DQMHistoTests: Total failures: 154
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555572
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

makortel · 2023-01-06T16:11:18Z

I think the annotations of class member variables in the class definition is a better way to express the information than using a separate file (classes_def.xml) or "magic comments". Thanks for finding it out!

@pcanal Can you think of any (potential) downsides from the ROOT point of view?

makortel · 2023-01-06T16:12:03Z

Comparison differences are in 11634.7 (#39803)

pcanal · 2023-01-06T19:49:11Z

Can you think of any (potential) downsides from the ROOT point of view?

Beside the need for the macro (that maybe we actually should be providing), the main risk is that we currently (i.e. as of right now, I just added it to my todo), we do not test using clang::annotate as a replace for the comments. (If it does not work for you right now, there is no reason for it to stop working unless the testing I add uncover some oddities that need a tweak in spelling)

fwyzard · 2023-01-06T19:58:07Z

It does work right now, and of course I do not mind making changes to the spelling if needed, or moving t centrally supplied macros.

One thing to note, though: I understand that the current version of cling is based on clang 9; more recent versions use (or at least support) a more general syntax for annotations.
So if cling moves to a more recent code base in the future, we may need to update the definition of the macros.

makortel · 2023-01-06T20:12:28Z

So if cling moves to a more recent code base in the future, we may need to update the definition of the macros.

ROOT 6.28 will come with LLVM 13 (and we already have that in the ROOT6_X IBs).

fwyzard · 2023-01-06T20:34:09Z

Can we trigger the tests for the ROOT6_X IB ?

makortel · 2023-01-06T20:37:59Z

@cmsbuild, please test for CMSSW_13_0_ROOT6_X

cmsbuild · 2023-01-07T12:49:33Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7fe410/29819/summary.html
COMMIT: 1c5c842
CMSSW: CMSSW_13_0_ROOT6_X_2023-01-05-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40435/29819/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
ROOTFileChecks: Some differences in event products or their sizes found
Reco comparison results: 16422 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555748
DQMHistoTests: Total failures: 38887
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3516839
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.258 KiB( 48 files compared)
DQMHistoSizes: changed ( 11834.0 ): 0.258 KiB SiStrip/MechanicalView
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: found differences in 6 / 47 workflows

fwyzard · 2023-01-07T15:02:31Z

OK, so the clang 13-based version of cling works, too.

fwyzard · 2023-01-07T15:03:21Z

I'd say it looks good to me - @makortel, let me know if you prefer different names for the macros or any other changes.

fwyzard · 2023-01-07T15:03:26Z

+heterogeneous

cmsbuild · 2023-01-10T16:39:33Z

Pull request #40435 was updated. @makortel, @smuzaffar, @Dr15Jones, @fwyzard can you please check and sign again.

cmsbuild · 2023-01-10T20:28:03Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7fe410/29885/summary.html
COMMIT: 56a348f
CMSSW: CMSSW_13_0_X_2023-01-10-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40435/29885/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test test-das-selected-lumis had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555538
DQMHistoTests: Total failures: 160
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555356
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

fwyzard · 2023-01-11T06:34:12Z

+heterogeneous

fwyzard · 2023-01-11T08:25:59Z

The only failed unit test I'd say is unrelated to these changes:

===== Test "test-das-selected-lumis" ====
+ export CMS_BOT_USE_DASGOCLIENT=true
+ CMS_BOT_USE_DASGOCLIENT=true
+ QUERY='lumi,file dataset=/HIHardProbes/HIRun2018A-v1/RAW run=326479'
+ grep '^/store/'
+ das-selected-lumis.py 1,23
+ dasgoclient --limit 0 --query 'lumi,file dataset=/HIHardProbes/HIRun2018A-v1/RAW run=326479' --format json
2023/01/10 17:56:28 ERROR failed to parse X509 proxy: crypto/tls: failed to parse key: asn1: syntax error: sequence truncated

---> test test-das-selected-lumis had ERRORS
TestTime:1
^^^^ End Test test-das-selected-lumis ^^^^

makortel · 2023-01-11T19:42:26Z

+core

Comparison failures are in 11634.7 (#39803)

cmsbuild · 2023-01-11T19:42:50Z

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

Axel-Naumann · 2023-01-12T06:53:17Z

Please note that the annotations used by rootcling / genreflex are currently an undocumented implementation detail and not part of ROOT's public API.

While root-project/root#12012 suggests to change that, I am fairly certain that the spelling of the attributes will change in the process. We are happy to work with you on a solution! Shall we discuss in the ROOT issue?

How urgent / important is this feature?

fwyzard · 2023-01-12T06:59:47Z

Hi @Axel-Naumann

Please note that the annotations used by rootcling / genreflex are currently an undocumented implementation detail and not part of ROOT's public API.

I know, it took some digging to figure it out :-D

I am fairly certain that the spelling of the attributes will change in the process.

No problem, it should be trivial to update the definition of the macros accordingly.

We are happy to work with you on a solution! Shall we discuss in the ROOT issue?

👍

How urgent / important is this feature?

It's... going in production for data taking in a couple months time :-)

Axel-Naumann · 2023-01-12T07:03:34Z

It's... going in production for data taking in a couple months time :-)

Well okay, for all means go for it :-) but that also means that making this an official interface isn't as urgent? "Just" important?

fwyzard · 2023-01-12T07:07:07Z

Well okay, for all means go for it :-) but that also means that making this an official interface isn't as urgent? "Just" important?

👍🏻

rappoccio · 2023-01-12T16:36:00Z

+1

Errors are unrelated to this PR

makortel · 2023-01-12T16:52:34Z

@rappoccio I believe this PR would need the explicit merge given the "failed" test report.

perrotta · 2023-01-12T16:55:52Z

merge

cmsbuild added this to the CMSSW_13_0_X milestone Jan 6, 2023

cmsbuild added code-checks-pending core-pending heterogeneous-pending orp-pending pending-signatures tests-started labels Jan 6, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 6, 2023

cmsbuild added tests-approved and removed tests-started labels Jan 6, 2023

cmsbuild added heterogeneous-approved and removed heterogeneous-pending labels Jan 7, 2023

cmsbuild added tests-rejected and removed tests-started labels Jan 10, 2023

cmsbuild added heterogeneous-approved and removed heterogeneous-pending labels Jan 11, 2023

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Jan 11, 2023

pcanal mentioned this pull request Jan 11, 2023

Add official support of C++ annotation to specify ROOT IO attribute for data members. root-project/root#12012

Open

cmsbuild added orp-approved and removed orp-pending labels Jan 12, 2023

cmsbuild merged commit a26cf49 into cms-sw:master Jan 12, 2023

fwyzard deleted the reflex_attributes branch March 9, 2023 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce macros to use reflex attributes #40435

Introduce macros to use reflex attributes #40435

fwyzard commented Jan 6, 2023 •

edited

fwyzard commented Jan 6, 2023

fwyzard commented Jan 6, 2023

fwyzard commented Jan 6, 2023

cmsbuild commented Jan 6, 2023

cmsbuild commented Jan 6, 2023

cmsbuild commented Jan 6, 2023

makortel commented Jan 6, 2023

makortel commented Jan 6, 2023

pcanal commented Jan 6, 2023

fwyzard commented Jan 6, 2023

makortel commented Jan 6, 2023

fwyzard commented Jan 6, 2023 via email

makortel commented Jan 6, 2023

cmsbuild commented Jan 7, 2023

fwyzard commented Jan 7, 2023

fwyzard commented Jan 7, 2023

fwyzard commented Jan 7, 2023

cmsbuild commented Jan 10, 2023

cmsbuild commented Jan 10, 2023

fwyzard commented Jan 11, 2023

fwyzard commented Jan 11, 2023

makortel commented Jan 11, 2023

cmsbuild commented Jan 11, 2023

Axel-Naumann commented Jan 12, 2023

fwyzard commented Jan 12, 2023

Axel-Naumann commented Jan 12, 2023

fwyzard commented Jan 12, 2023

rappoccio commented Jan 12, 2023

makortel commented Jan 12, 2023

perrotta commented Jan 12, 2023

Introduce macros to use reflex attributes #40435

Introduce macros to use reflex attributes #40435

Conversation

fwyzard commented Jan 6, 2023 • edited

PR description:

PR validation:

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

fwyzard commented Jan 6, 2023

fwyzard commented Jan 6, 2023

fwyzard commented Jan 6, 2023

cmsbuild commented Jan 6, 2023

cmsbuild commented Jan 6, 2023

cmsbuild commented Jan 6, 2023

Comparison Summary

makortel commented Jan 6, 2023

makortel commented Jan 6, 2023

pcanal commented Jan 6, 2023

fwyzard commented Jan 6, 2023

makortel commented Jan 6, 2023

fwyzard commented Jan 6, 2023 via email

makortel commented Jan 6, 2023

cmsbuild commented Jan 7, 2023

Comparison Summary

fwyzard commented Jan 7, 2023

fwyzard commented Jan 7, 2023

fwyzard commented Jan 7, 2023

cmsbuild commented Jan 10, 2023

cmsbuild commented Jan 10, 2023

Unit Tests

Comparison Summary

fwyzard commented Jan 11, 2023

fwyzard commented Jan 11, 2023

makortel commented Jan 11, 2023

cmsbuild commented Jan 11, 2023

Axel-Naumann commented Jan 12, 2023

fwyzard commented Jan 12, 2023

Axel-Naumann commented Jan 12, 2023

fwyzard commented Jan 12, 2023

rappoccio commented Jan 12, 2023

makortel commented Jan 12, 2023

perrotta commented Jan 12, 2023

fwyzard commented Jan 6, 2023 •

edited