[ntuple] Support the storage of collections using the `TVirtualCollectionProxy` interface #11525

jalopezg-git · 2022-10-10T11:55:06Z

This pull request introduces RField support for user-defined classes that behave as collections of elements. These classes specify a "collection proxy" that provides access to the elements in the collection.
A legacy collection proxy is provided in the form of an instance of a class derived from TVirtualCollectionProxy. The collection proxy for a class is typically set during initialization, usually using TClass::CopyCollectionProxy() or similar.

In this pull request, we introduce support for legacy collection proxies derived from the TVirtualCollectionProxy class, although in later stages, RNTuple might provide this functionality through an interface different interface.

At a bare minimum, the user is required to provide an implementation for the following functions in TVirtualCollectionProxy:

HasPointers(), GetProperties(), GetValueClass(), GetType(), Sizeof(): for general information about the collection and its value type.
Sizeof(), At(), Clear(), and Insert(): for element access
PushProxy(), PopProxy(): for selecting the target object.

A usage example can be seen here.

Changes or fixes:

Added class RCollectionClassField, representing a field of a user-defined class that behaves as a collection that is compliant with the TVirtualCollectionProxy interface.
Given that the RField<T> primary template definition (that used if no specialization matches) uses RClassField to store members of a class, we rely on an additional helper type (IsCollectionProxy<T>) to annotate that a particular class has an associated collection proxy (see example below).

// Alternatively, this can be specified via a member type in the user-defined class; see the documentation
template <>
struct IsCollectionProxy<ProxiedCollection> : std::true_type {};

auto model = RNTupleModel::Create();
auto fieldKlass = model->MakeField<MyClass>("fieldKlass"); // Regular class
auto fieldCollection = model->MakeField<ProxiedCollection>("fieldCollection"); // Class with associated collection proxy

This tag is not required when using the type-erased interface (i.e., RFieldBase::Create()), as the use of a collection proxy can be determined at run time via TClass.

RCollectionClassField::ReadGlobalImpl(): reduce the number of virtual calls to TVirtualCollectionProxy::Insert() by reading and inserting a block of elements at once. The size of the buffer that holds the elements is set by RCollectionClassField::kReadChunkSize (in bytes).
Provide implementation and tests for RPrintValueVisitor::VisitCollectionClassField()
Add unit tests for collections of fundamental types, compound types, vector of a collection proxy, and nested collection proxies.
Update doc/specifications.md

Checklist:

tested changes locally
updated the docs (if necessary)

This PR fixes #11523.

Initial skeleton for supporting classes with an associated collection proxy. Object of such type behave as a collection of elements accessible through the `TVirtualCollectionProxy` interface.

…unctions

Reduce the number of virtual calls to `TVirtualCollectionProxy::Insert()` by reading and inserting a block of elements at once.

phsft-bot · 2022-10-10T11:55:12Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

jalopezg-git · 2022-10-10T11:59:51Z

FYI, @mnowakgit 🙂

phsft-bot · 2022-10-10T12:05:08Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-10-10T12:04:15.629Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ROOT.util

phsft-bot · 2022-10-10T14:16:23Z

Build failed on mac1015/cxx17.
Running on macitois22.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

[2022-10-10T13:22:28.445Z] /Volumes/HD2/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/test/ntuple_types.cxx:440:9: warning: 'Insert' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
[2022-10-10T13:22:28.445Z] /Volumes/HD2/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/test/ntuple_types.cxx:440:9: warning: 'Insert' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
[2022-10-10T13:22:28.446Z] /Volumes/HD2/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/test/ntuple_types.cxx:440:9: warning: 'Insert' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

phsft-bot · 2022-10-10T14:50:19Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-10-10T14:59:49Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-10-10T14:59:47.973Z] FAILED: lib/modules.idx

jblomer

That is awesome!

Some minor comments inline; for the tests: let's also test: vector of collection proxies, collection proxy of vectors and collection proxy of collection proxies.

tree/ntuple/v7/inc/ROOT/RNTupleView.hxx

tree/ntuple/v7/inc/ROOT/RFieldVisitor.hxx

tree/ntuple/v7/inc/ROOT/RField.hxx

tree/ntuple/v7/src/RField.cxx

tree/ntuple/v7/inc/ROOT/RField.hxx

tree/ntuple/v7/src/RField.cxx

…emplate arg Do not rely on the use of an additional "tag" argument to disambiguate between the `RField` primary template definition that derives from `RClassField` and the specialization for classes using a collection proxy. Instead, this commit introduces the `IsCollectionProxy<T>` template that can be specialized to mark a specific class as a proxied collection.

phsft-bot · 2022-10-17T14:45:18Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

Test the usage of collection proxies yielding elements of a fundamental C++ type.

Test the usage of collection proxies whose value type is a compound type with nested collections.

Co-authored-by: Jakob Blomer <jblomer@cern.ch>

…r type

…bytes

phsft-bot · 2022-10-17T15:01:47Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

- Test `RField<std::vector<T>>` where `T` is a proxied collection - Test nested collection proxies

tree/ntuple/v7/src/RField.cxx

tree/ntuple/v7/test/CustomStruct.hxx

phsft-bot · 2022-10-17T20:51:46Z

Build failed on mac1015/cxx17.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.gui.webdisplay.test.test_webgui_ping

…roxy Co-authored-by: Philippe Canal <pcanal@fnal.gov>

phsft-bot · 2022-10-18T09:59:27Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

jblomer

Many thanks for the update! Not entirely through yet but some comments are ready for discussion.

tree/ntuple/v7/src/RField.cxx

tree/ntuple/v7/test/CustomStruct.hxx

tree/ntuple/v7/test/SimpleCollectionProxy.hxx

phsft-bot · 2022-10-25T10:44:50Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-10-25T10:49:04Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-11-07T12:33:47Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-11-07T14:43:49Z

Build failed on mac1015/cxx17.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.gui.webdisplay.test.test_webgui_ping

`RClassField` and `RCollectionClassField` (used for regular user-defined classes / classes with an associated collection proxy, respectively) should throw if used unappropriately, i.e. if `RClassField` is instantiated to handle a class that has a collection proxy set or viceversa.

phsft-bot · 2022-11-08T12:13:44Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

jblomer

Great work! From my side, that's good to go. It would probably be a good idea to still address Philippe's comment on `fProxy->Clear();.

tree/ntuple/v7/src/RField.cxx

Instead of manually destroying collection items in `ReadGlobalImpl()`, rely on `TVirtualCollectionProxy::Clear()`. Rationale (a PR comment by jblomer): The `DestroyValue` and `GenerateValue` calls should be symmetric. So if the collection proxy takes care of cleaning memory, it should also be in charge of (default)-constructing new elements on resize. I think that is the case here. The `GenerateValue` call is only used on the internal buffer. The collection then copies the elements from the internal buffer into its own memory, IIUC. Co-authored-by: Philippe Canal <pcanal@fnal.gov> Co-authored-by: Jakob Blomer <jblomer@cern.ch>

phsft-bot · 2022-11-10T11:14:14Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

pcanal

Thanks.

jalopezg-git added 3 commits October 10, 2022 13:14

[ntuple] Add skeleton for RCollectionClassField

7e5a42b

Initial skeleton for supporting classes with an associated collection proxy. Object of such type behave as a collection of elements accessible through the `TVirtualCollectionProxy` interface.

[ntuple] Prototype implementation of RCollectionClassField member f…

2663b83

…unctions

[ntuple] RCollectionClassField::ReadGlobalImpl(): implement chunked read

9febf99

Reduce the number of virtual calls to `TVirtualCollectionProxy::Insert()` by reading and inserting a block of elements at once.

jalopezg-git added new feature in:RNTuple labels Oct 10, 2022

jalopezg-git requested a review from pcanal October 10, 2022 11:55

jalopezg-git requested a review from jblomer as a code owner October 10, 2022 11:55

jalopezg-git self-assigned this Oct 10, 2022

jalopezg-git force-pushed the ntuple-tvirtualcollectionproxy branch from 429dc07 to 129332b Compare October 10, 2022 14:50

jblomer requested changes Oct 12, 2022

View reviewed changes

jalopezg-git force-pushed the ntuple-tvirtualcollectionproxy branch from 129332b to c0a8676 Compare October 17, 2022 14:45

jalopezg-git and others added 5 commits October 17, 2022 17:00

[ntuple] RCollectionClassField: test use with fundamental types

a8bcced

Test the usage of collection proxies yielding elements of a fundamental C++ type.

[ntuple] RCollectionClassField: test use with compound types

61e4787

Test the usage of collection proxies whose value type is a compound type with nested collections.

[ntuple] RCollectionClassField: improve error messages/documentation

6b7516b

Co-authored-by: Jakob Blomer <jblomer@cern.ch>

[ntuple] RField: allow the use of IsCollectionProxy also as a membe…

6bb3db5

…r type

[ntuple] RCollectionClassField: set ReadGlobalImpl() chunk size in …

f6e21a2

…bytes

jalopezg-git force-pushed the ntuple-tvirtualcollectionproxy branch from c0a8676 to f72e321 Compare October 17, 2022 15:01

jalopezg-git added 3 commits October 17, 2022 17:08

[ntuple] RCollectionClassField: add extended test cases

5b59b68

- Test `RField<std::vector<T>>` where `T` is a proxied collection - Test nested collection proxies

[ntuple] Allow value printing of classes using a collection proxy

2208bee

[ntuple] Test value printing of classes using a collection proxy

377a520

jalopezg-git force-pushed the ntuple-tvirtualcollectionproxy branch from f72e321 to 377a520 Compare October 17, 2022 15:09

pcanal reviewed Oct 17, 2022

View reviewed changes

tree/ntuple/v7/src/RField.cxx Show resolved Hide resolved

pcanal reviewed Oct 17, 2022

View reviewed changes

tree/ntuple/v7/src/RField.cxx Outdated Show resolved Hide resolved

pcanal reviewed Oct 17, 2022

View reviewed changes

tree/ntuple/v7/src/RField.cxx Outdated Show resolved Hide resolved

pcanal reviewed Oct 17, 2022

View reviewed changes

tree/ntuple/v7/src/RField.cxx Show resolved Hide resolved

pcanal reviewed Oct 17, 2022

View reviewed changes

tree/ntuple/v7/test/CustomStruct.hxx Show resolved Hide resolved

[ntuple] RCollectionClassField: adopt a clone of a class collection p…

67748bd

…roxy Co-authored-by: Philippe Canal <pcanal@fnal.gov>

jblomer requested changes Oct 19, 2022

View reviewed changes

tree/ntuple/v7/src/RField.cxx Show resolved Hide resolved

tree/ntuple/v7/test/CustomStruct.hxx Show resolved Hide resolved

tree/ntuple/v7/test/SimpleCollectionProxy.hxx Show resolved Hide resolved

jalopezg-git mentioned this pull request Oct 20, 2022

[ntuple] Check the usage of Long_t in RField (size of the type might differ between 32- and 64-bit platforms) #11616

Open

1 task

[ntuple] Add comment on rationale of IsCollectionProxy<T> trait

e926475

jalopezg-git force-pushed the ntuple-tvirtualcollectionproxy branch from 232fb0f to e926475 Compare October 25, 2022 10:48

jalopezg-git requested a review from jblomer October 25, 2022 10:55

jalopezg-git requested a review from pcanal November 7, 2022 11:17

jalopezg-git added 2 commits November 8, 2022 12:45

[skip-ci][ntuple] Update doc/specifications.md

e619d57

jalopezg-git force-pushed the ntuple-tvirtualcollectionproxy branch from 04f7b88 to e619d57 Compare November 8, 2022 12:13

jalopezg-git mentioned this pull request Nov 8, 2022

[ntuple] Pending optimizations for storage of classes with an associated collection proxy #11671

Closed

3 tasks

jblomer approved these changes Nov 9, 2022

View reviewed changes

tree/ntuple/v7/src/RField.cxx Outdated Show resolved Hide resolved

jalopezg-git merged commit 072f91d into root-project:master Nov 10, 2022

jalopezg-git deleted the ntuple-tvirtualcollectionproxy branch November 10, 2022 20:24

pcanal reviewed Nov 11, 2022

View reviewed changes

jalopezg-git mentioned this pull request Feb 24, 2023

[ntuple] Improve support for classes with an associated collection proxy #12380

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ntuple] Support the storage of collections using the `TVirtualCollectionProxy` interface #11525

[ntuple] Support the storage of collections using the `TVirtualCollectionProxy` interface #11525

jalopezg-git commented Oct 10, 2022 •

edited

Loading

phsft-bot commented Oct 10, 2022

jalopezg-git commented Oct 10, 2022

phsft-bot commented Oct 10, 2022

phsft-bot commented Oct 10, 2022

phsft-bot commented Oct 10, 2022

phsft-bot commented Oct 10, 2022

jblomer left a comment

phsft-bot commented Oct 17, 2022

phsft-bot commented Oct 17, 2022

phsft-bot commented Oct 17, 2022

phsft-bot commented Oct 18, 2022

jblomer left a comment •

edited

Loading

phsft-bot commented Oct 25, 2022

phsft-bot commented Oct 25, 2022

phsft-bot commented Nov 7, 2022

phsft-bot commented Nov 7, 2022

phsft-bot commented Nov 8, 2022

jblomer left a comment

phsft-bot commented Nov 10, 2022

pcanal left a comment

[ntuple] Support the storage of collections using the TVirtualCollectionProxy interface #11525

[ntuple] Support the storage of collections using the TVirtualCollectionProxy interface #11525

Conversation

jalopezg-git commented Oct 10, 2022 • edited Loading

Changes or fixes:

Checklist:

phsft-bot commented Oct 10, 2022

jalopezg-git commented Oct 10, 2022

phsft-bot commented Oct 10, 2022

Errors:

phsft-bot commented Oct 10, 2022

Warnings:

phsft-bot commented Oct 10, 2022

phsft-bot commented Oct 10, 2022

Errors:

jblomer left a comment

Choose a reason for hiding this comment

phsft-bot commented Oct 17, 2022

phsft-bot commented Oct 17, 2022

phsft-bot commented Oct 17, 2022

Failing tests:

phsft-bot commented Oct 18, 2022

jblomer left a comment • edited Loading

Choose a reason for hiding this comment

phsft-bot commented Oct 25, 2022

phsft-bot commented Oct 25, 2022

phsft-bot commented Nov 7, 2022

phsft-bot commented Nov 7, 2022

Failing tests:

phsft-bot commented Nov 8, 2022

jblomer left a comment

Choose a reason for hiding this comment

phsft-bot commented Nov 10, 2022

pcanal left a comment

Choose a reason for hiding this comment

[ntuple] Support the storage of collections using the `TVirtualCollectionProxy` interface #11525

[ntuple] Support the storage of collections using the `TVirtualCollectionProxy` interface #11525

jalopezg-git commented Oct 10, 2022 •

edited

Loading

jblomer left a comment •

edited

Loading