Implement ROOT read rules for PortableHostCollection #43219

fwyzard · 2023-11-07T18:13:01Z

PR description:

Implement ROOT read rules for PortableHostCollection instantiations.

These replace the user-defined read rules in the XML file

  <read
    sourceClass="portabletest::TestHostCollection"
    targetClass="portabletest::TestHostCollection"
    version="[1-]"
    source="portabletest::TestSoA layout_;"
    target="buffer_,layout_,view_"
    embed="false">
  <![CDATA[
    portabletest::TestHostCollection::ROOTReadStreamer(newObj, onfile.layout_);
  ]]>
  </read>

with a macro-based C++ implementation in DataFormats/PortableTestObjects/src/classes.cc:

#include "DataFormats/Portable/interface/PortableHostCollectionReadRules.h"
#include "DataFormats/PortableTestObjects/interface/TestHostCollection.h"

SET_PORTABLEHOSTCOLLECTION_READ_RULES(portabletest::TestHostCollection);

PR validation:

The HeterogeneousCore/AlpakaTest/test/testHeterogeneousCoreAlpakaTestWriteRead.sh unit test pass:

the portabletest::TestHostCollection is written to a ROOT file;
it is read back from the ROOT file using the user-defined streamer function;
the validation checks pass.

fwyzard · 2023-11-07T18:15:23Z

DataFormats/PortableTestObjects/src/classes.cc

+   rule->fSource = "portabletest::TestHostCollection::Layout layout_;";
+   rule->fTarget = "";
+   rule->fFunctionPtr = (void *)TFunc2void(setROOTReadStreamerFor_portabletest_TestHostCollection);
+   rule->fCode = "";


This is could be set to "portabletest::TestHostCollection::ROOTReadStreamer(newObj, onfile.layout_);".
For the moment it is left empty on purpose, in case we decide to modify setROOTReadStreamerFor_portabletest_TestHostCollection() and portabletest::TestHostCollection::ROOTReadStreamer() in a way that cannot be reflected in the XML read rule.

fwyzard · 2023-11-07T18:16:07Z

DataFormats/PortableTestObjects/src/classes.cc

@@ -0,0 +1,52 @@
+#define G__DICTIONARY


This is required by RtypesImp.h, which is needed for TFunc2void.

fwyzard · 2023-11-07T18:18:22Z

DataFormats/PortableTestObjects/src/classes.cc

+
+// read function for portabletest::TestHostCollection
+// called for every event
+static void setROOTReadStreamerFor_portabletest_TestHostCollection(char *target, TVirtualObject *oldObj)


This needs a better name :-)

Would it be a good idea to make it a (static or regular) member function of PortableHostCollection ?

It could simplify the interface, but the main downside I can think of is that it would add a dependency on the ROOT internals to the CMS type.

I'd avoid making PortableHostCollection dependent on ROOT.

cmsbuild · 2023-11-07T18:19:21Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43219/37560

This PR adds an extra 16KB 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, Porting Pixel Tracks to Alpaka [Not to Merge] #41117

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43219/37560/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43219/37560/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

fwyzard · 2023-11-07T18:21:05Z

@makortel @ericcano @pcanal here is what I've come up with so far, base on the discussion we had this afternoon.

The idea would be to move as much as possible of the functionality to a set of macros or templates, so that the user would need to implement as little as possible.

Comments and suggestions are welcome.

By the way, can you think of any way to do this without requiring a new .cc file ?

fwyzard · 2023-11-07T18:22:10Z

DataFormats/PortableTestObjects/src/classes_def.xml

    sourceClass="portabletest::TestHostCollection"
    targetClass="portabletest::TestHostCollection"
    version="[1-]"
    source="portabletest::TestSoA layout_;"
-    target="buffer_,layout_,view_"
+    target=""


I think I misunderstood how the target directive works.
After reading the code generated by ROOT, I don't think we need to specify these, even if we were to stay with the XML approach.

I though the content of target were needed to avoid memory leaks, and tell ROOT this read rule needs to be run if any of those member variables are asked for (#40491, #40492).

They definitively are needed. They may not change the content of the generated function but they affect significantly the way TStreamerInfo handles the rules. As Matti said, not specifying will lead to 'huge' memory leak.

Can you show me what is affected by the definition of the target entry ?

It is a bit complicated and indirect but you can start by looking the consequence of those lines:

io/io/src/TStreamerInfo.cxx 639: && rules && !rules.HasRuleWithTarget( element->GetName(), kTRUE ) ) 2508: && !rules.HasRuleWithTarget( element->GetName(), kTRUE ) ) { 2530: } else if (rules.HasRuleWithTarget( element->GetName(), kTRUE ) ) { 2542: } else if (rules && rules.HasRuleWithTarget( element->GetName(), kTRUE ) ) { io/io/src/TStreamerInfo.cxx 4548: if ( rule->HasTarget( element->GetName() ) ) { io/io/src/TStreamerInfo.cxx 1850: const TObjArray* targets = rule->GetTarget(); 4595: if (rule->GetTarget()==0) { 4607: toAdd.reserve(rule->GetTarget()->GetEntriesFast()); 4608: TObjString * objstr = (TObjString*)(rule->GetTarget()->At(0)); 4625: for(Int_t other = 1; other < rule->GetTarget()->GetEntriesFast(); ++other) { 4626: objstr = (TObjString*)(rule->GetTarget()->At(other));

In particular line 639, 2508, 4548, 4626 or even 4595

Thanks.

So the effect is not in the code generated by the read rules, but deeper in TStreamerInfo.

Yes, exactly.

fwyzard · 2023-11-07T18:23:45Z

please test

cmsbuild · 2023-11-07T18:28:20Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43219/37561

This PR adds an extra 16KB 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, Porting Pixel Tracks to Alpaka [Not to Merge] #41117

cmsbuild · 2023-11-07T18:28:46Z

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

It involves the following packages:

DataFormats/PortableTestObjects (heterogeneous)

@makortel, @fwyzard can you please review it and eventually sign? Thanks.
@rovere, @missirol this is something you requested to watch as well.
@sextonkennedy, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2023-11-07T21:01:41Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b543d1/35676/summary.html
COMMIT: ca26006
CMSSW: CMSSW_14_0_X_2023-11-07-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43219/35676/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 23 lines to the logs
Reco comparison results: 134 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3363010
DQMHistoTests: Total failures: 1789
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3361199
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2023-11-08T08:17:46Z

Comparison differences are related to #39803

fwyzard · 2023-11-09T17:34:26Z

please test

fwyzard · 2023-11-09T17:34:32Z

+heterogeneous

cmsbuild · 2023-11-09T17:41:35Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43219/37596

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File DataFormats/Portable/README.md modified in PR(s): Add support to multi layout portable collections (any number) #40285
- File DataFormats/Portable/interface/PortableHostCollection.h modified in PR(s): Add support to multi layout portable collections (any number) #40285
- File DataFormats/PortableTestObjects/src/classes_def.xml modified in PR(s): Add support to multi layout portable collections (any number) #40285, Porting Pixel Tracks to Alpaka [Not to Merge] #41117

cmsbuild · 2023-11-09T17:41:57Z

Pull request #43219 was updated. can you please check and sign again.

cmsbuild · 2023-11-09T21:57:51Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b543d1/35733/summary.html
COMMIT: ceab451
CMSSW: CMSSW_14_0_X_2023-11-09-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43219/35733/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially removed 50 lines from the logs
Reco comparison results: 28 differences found in the comparisons
DQMHistoTests: Total files compared: 50
DQMHistoTests: Total histograms compared: 3363010
DQMHistoTests: Total failures: 1405
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3361583
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
Checked 214 log files, 167 edm output root files, 50 DQM output files
TriggerResults: no differences found

makortel · 2023-11-10T12:29:41Z

Comparison differences are related to #39803

rappoccio · 2023-11-10T14:44:18Z

+1

cmsbuild added this to the CMSSW_14_0_X milestone Nov 7, 2023

cmsbuild added pending-signatures tests-pending orp-pending code-checks-pending heterogeneous-pending labels Nov 7, 2023

fwyzard commented Nov 7, 2023

View reviewed changes

cmsbuild added code-checks-rejected and removed code-checks-pending labels Nov 7, 2023

fwyzard commented Nov 7, 2023

View reviewed changes

cmsbuild added code-checks-pending and removed code-checks-rejected labels Nov 7, 2023

cmsbuild added tests-started and removed tests-pending labels Nov 7, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 7, 2023

cmsbuild added tests-approved and removed tests-started labels Nov 7, 2023

fwyzard force-pushed the test_user_defined_ROOT_streamer branch from ca26006 to 39e4240 Compare November 8, 2023 14:32

cmsbuild removed tests-approved code-checks-approved labels Nov 8, 2023

fwyzard force-pushed the test_user_defined_ROOT_streamer branch from 4e52d51 to ceab451 Compare November 9, 2023 17:34

fwyzard marked this pull request as ready for review November 9, 2023 17:34

cmsbuild added code-checks-pending and removed code-checks-approved labels Nov 9, 2023

cmsbuild added fully-signed tests-started heterogeneous-approved and removed pending-signatures tests-pending heterogeneous-pending labels Nov 9, 2023

fwyzard changed the title ~~Test a user-defined implementation of the ROOT read rules~~ Implement user-defined ROOT read rules for PortableHostCollection Nov 9, 2023

fwyzard changed the title ~~Implement user-defined ROOT read rules for PortableHostCollection~~ Implement user-defined read rules for PortableHostCollection Nov 9, 2023

fwyzard changed the title ~~Implement user-defined read rules for PortableHostCollection~~ Implement ROOT read rules for PortableHostCollection Nov 9, 2023

fwyzard mentioned this pull request Nov 9, 2023

Implement ROOT read rules for PortableHostCollection [13.3.x] #43241

Merged

cmsbuild added code-checks-approved and removed code-checks-pending labels Nov 9, 2023

cmsbuild added tests-approved and removed tests-started labels Nov 9, 2023

cmsbuild added orp-approved and removed orp-pending labels Nov 10, 2023

cmsbuild merged commit 2db9ea8 into cms-sw:master Nov 10, 2023

cmsbuild mentioned this pull request Nov 11, 2023

Implementation of DeepTau SONIC producers #43158

Merged

fwyzard deleted the test_user_defined_ROOT_streamer branch January 30, 2024 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ROOT read rules for PortableHostCollection #43219

Implement ROOT read rules for PortableHostCollection #43219

fwyzard commented Nov 7, 2023 •

edited

Loading

fwyzard Nov 7, 2023

fwyzard Nov 7, 2023

fwyzard Nov 7, 2023

makortel Nov 8, 2023

cmsbuild commented Nov 7, 2023

fwyzard commented Nov 7, 2023

fwyzard Nov 7, 2023

makortel Nov 8, 2023

pcanal Nov 8, 2023

fwyzard Nov 8, 2023

pcanal Nov 8, 2023

fwyzard Nov 8, 2023

pcanal Nov 8, 2023

fwyzard commented Nov 7, 2023

cmsbuild commented Nov 7, 2023

cmsbuild commented Nov 7, 2023

cmsbuild commented Nov 7, 2023

makortel commented Nov 8, 2023

fwyzard commented Nov 9, 2023

fwyzard commented Nov 9, 2023

cmsbuild commented Nov 9, 2023

cmsbuild commented Nov 9, 2023

cmsbuild commented Nov 9, 2023

makortel commented Nov 10, 2023

rappoccio commented Nov 10, 2023

Implement ROOT read rules for PortableHostCollection #43219

Implement ROOT read rules for PortableHostCollection #43219

Conversation

fwyzard commented Nov 7, 2023 • edited Loading

PR description:

PR validation:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmsbuild commented Nov 7, 2023

fwyzard commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwyzard commented Nov 7, 2023

cmsbuild commented Nov 7, 2023

cmsbuild commented Nov 7, 2023

cmsbuild commented Nov 7, 2023

Comparison Summary

makortel commented Nov 8, 2023

fwyzard commented Nov 9, 2023

fwyzard commented Nov 9, 2023

cmsbuild commented Nov 9, 2023

cmsbuild commented Nov 9, 2023

cmsbuild commented Nov 9, 2023

Comparison Summary

makortel commented Nov 10, 2023

rappoccio commented Nov 10, 2023

fwyzard commented Nov 7, 2023 •

edited

Loading