[ntuple] Add RRVecField for type-erased I/O of RVecs by eguiraud · Pull Request #8770 · root-project/root

eguiraud · 2021-07-29T12:39:19Z

This PR:

adds the RRVecField type for type-erased I/O of RVecs with RNTuple
removes some special-casing of RVec<bool> which is unnecessary now that we have RVec 2.0
adds RPrintValueVisitor support for RVecs
adds tests for type-erased I/O of RVecs
adds tests for ntuple->Show + RVecs
adds tests for interop I/O of std::vectors and RVecs (i.e. writing one and reading the other)

Checklist:

tested changes locally
updated the docs (if necessary): I don't think it's necessary

This PR fixes #6347 .

phsft-bot · 2021-07-29T12:39:28Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

ghost · 2021-07-29T12:39:30Z

DeepCode's analysis on #47e2b3 found:

⚠️ 2 warnings 👇

Top issues

Description	Example fixes
The result of malloc, which may return null flows to the first argument of memcpy. This could result in undefined behavior. Consider adding a check for nullness. Occurrences: RVec.cxx:65	🔧 Example fixes
Memory leak. Memory is never freed if realloc fails to allocate memory. Occurrences: RVec.cxx:68	🔧 Example fixes

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

If you are using our plugins, you might be interested in their successors: Snyk's JetBrains plugin and Snyk's VS Code plugin.

phsft-bot · 2021-07-29T13:46:07Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

jblomer

Awesome! Some comments inline. I marked some std::int32_t instead of int32_t but not all. I think it would be also good to warn in the documentation of RVec that changes to the memory allocation and to the class layout need to be reflected here (perhaps that's already there).

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

tree/ntuple/v7/test/rfield_vector.cxx

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

tree/ntuple/v7/src/RField.cxx

phsft-bot · 2021-07-30T09:53:22Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-30T09:55:34Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-30T11:01:36Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

phsft-bot · 2021-07-30T15:58:56Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-30T16:01:43Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-30T16:08:07Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-07-30T16:48:18Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-07-30T16:09:18.100Z] stderr: error: could not read '.git/rebase-apply/head-name': No such file or directory

Failing tests:

projectroot.roottest.python.cpp.roottest_python_cpp_cpp

tree/ntuple/v7/src/RField.cxx

phsft-bot · 2021-10-07T16:01:35Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

eguiraud · 2021-10-07T16:02:37Z

The only major things left should be the extra tests required by @jblomer and support for RPrintValueVisitor. Let's see if jenkins agrees.

phsft-bot · 2021-10-07T17:36:14Z

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

Warnings:

[2021-10-07T16:41:18.314Z] builtins/xrootd/XROOTD-prefix/include/xrootd/XrdCl/XrdClOptional.hh:58:29: warning: unused parameter 'n' [-Wunused-parameter]

phsft-bot · 2021-10-07T19:45:55Z

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

Warnings:

[2021-10-07T18:57:06.763Z] builtins/xrootd/XROOTD-prefix/include/xrootd/XrdCl/XrdClOptional.hh:58:29: warning: unused parameter 'n' [-Wunused-parameter]

Failing tests:

phsft-bot · 2021-10-08T18:04:48Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2021-10-08T19:15:00Z

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

Failing tests:

phsft-bot · 2021-10-11T17:12:19Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

phsft-bot · 2022-04-14T16:22:52Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2022-04-14T16:20:57.573Z] C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\tools\clang\include\clang/AST/Decl.h(2112,1): fatal error C1088: Cannot flush compiler intermediate file: 'C:\Users\sftnight\AppData\Local\Temp_CL_150a260dex': Invalid argument (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\tools\clang\lib\ASTMatchers\ASTMatchFinder.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\tools\clang\lib\ASTMatchers\obj.clangASTMatchers.vcxproj]
[2022-04-14T16:21:11.690Z] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\vector(739,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Target\NVPTX\NVPTXPeephole.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Target\NVPTX\LLVMNVPTXCodeGen.vcxproj]
[2022-04-14T16:21:11.691Z] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\xutility(1208,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\IPO\FunctionImport.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\IPO\LLVMipo.vcxproj]
[2022-04-14T16:21:11.691Z] C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Analysis\TypeBasedAliasAnalysis.cpp(741,1): fatal error C1060: compiler is out of heap space [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Analysis\LLVMAnalysis.vcxproj]
[2022-04-14T16:21:11.691Z] C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\include\llvm/IR/Metadata.h(496,19): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Target\NVPTX\NVPTXAssignValidGlobalNames.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Target\NVPTX\LLVMNVPTXCodeGen.vcxproj]
[2022-04-14T16:21:11.691Z] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\tuple(190,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\IPO\Attributor.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\IPO\LLVMipo.vcxproj]
[2022-04-14T16:21:11.691Z] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\tuple(494,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Analysis\ScalarEvolutionNormalization.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Analysis\LLVMAnalysis.vcxproj]
[2022-04-14T16:21:11.691Z] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\ratio(78,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Target\NVPTX\NVPTXTargetTransformInfo.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Target\NVPTX\LLVMNVPTXCodeGen.vcxproj]
[2022-04-14T16:21:11.691Z] C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\include\llvm/ADT/ArrayRef.h(184,1): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Transforms\IPO\SCCP.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Transforms\IPO\LLVMipo.vcxproj]
[2022-04-14T16:21:11.691Z] C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\ratio(28,53): fatal error C1060: compiler is out of heap space (compiling source file C:\build\workspace\root-pullrequests-build\root\interpreter\llvm\src\lib\Target\NVPTX\NVPTXReplaceImageHandles.cpp) [C:\build\workspace\root-pullrequests-build\build\interpreter\llvm\src\lib\Target\NVPTX\LLVMNVPTXCodeGen.vcxproj]

And 365 more

eguiraud · 2022-04-22T13:25:36Z

@jblomer about the discussion at #8770 (comment) , I remember that for RVec we said we would not call destructors for collection elements at every entry, but I see that RVectorField does at line 1039?

root/tree/ntuple/v7/src/RField.cxx

Lines 1036 to 1044 in 7e709f8

    
           auto oldNItems = typedValue->size() / fItemSize; 
        
           for (std::size_t i = nItems; i < oldNItems; ++i) { 
        
              auto itemValue = fSubFields[0]->CaptureValue(typedValue->data() + (i * fItemSize)); 
        
              fSubFields[0]->DestroyValue(itemValue, true /* dtorOnly */); 
        
           } 
        
           typedValue->resize(nItems * fItemSize); 
        
           for (std::size_t i = oldNItems; i < nItems; ++i) { 
        
              fSubFields[0]->GenerateValue(typedValue->data() + (i * fItemSize)); 
        
           }

Also if the resize at line 1041 needs to reallocate is it correct to only call GenerateValue for new elements? If my understanding is correct, the bytes of the old elements will be correct, but no placement new for a value of the correct type will have been called in the new memory location so technically the object's lifetimes is not started.

(asking about RVectorField to understand what to do with RRVecField)

phsft-bot · 2022-04-22T16:31:26Z

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

jblomer · 2022-05-04T09:28:30Z

@jblomer about the discussion at #8770 (comment) , I remember that for RVec we said we would not call destructors for collection elements at every entry, but I see that RVectorField does at line 1039?

It does so only for the left-over elements when the collection shrinks.

Also if the resize at line 1041 needs to reallocate is it correct to only call GenerateValue for new elements? If my understanding is correct, the bytes of the old elements will be correct, but no placement new for a value of the correct type will have been called in the new memory location so technically the object's lifetimes is not started.

Good point! I created #10520.

eguiraud · 2022-05-04T09:36:46Z

It does so only for the left-over elements when the collection shrinks.

uhm why are leftover elements different from elements that are replaced? if some resource management logic has to be called when an element is removed, it might also have to be called when it is replaced?

EDIT: ah, I see now that #10520 basically covers this

jblomer

Looks good to me, except one tiny comment on the change in the RNTupleModel API

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

eguiraud · 2022-05-18T14:46:28Z

Sorry I have to rebase this on top of master again in order to be able to compile the branch with gcc 12..

With RVec's redesign, RVec<bool> now behaves consistently with all other RVec<T>'s (it does not inherit vector<bool>'s behavior).

This removes some code duplication and makes it possible for specialized RField<RVec<T>> instances to act as general RRVecField instances for what regards visitor implementations (immediately useful to implement a single RPrintValueVisitor for all RVec fields).

Plus tests for type-erased and non-type-erased value printing.

RNTuple does not explicitly destroy existing values when reading, it only overwrites the bytes.

Co-authored-by: Philippe Canal <[email protected]>

When reading RVecs, we now: - destroy excess elements if size is reduced w.r.t. the previous entry, for consistency with std::vector I/O - generate new values for new elements if size is increased w.r.t. the previous entry -- only if needed instead of unconditionally as before - when reallocating, we call destructors on the old elements and generate the corresponding new elements in the new memory buffer

phsft-bot · 2022-05-19T08:18:14Z

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

eguiraud · 2022-05-19T08:20:21Z

This should be ready to go, with two performance optimizations (marked as TODOs) that are left for future PRs (this one is already terribly large, I think).

jblomer

Great! I'm looking forward to adjusting the ntuple data source to this.

eguiraud requested a review from jblomer July 29, 2021 12:39

eguiraud self-assigned this Jul 29, 2021

jblomer requested changes Jul 29, 2021

View reviewed changes

This was referenced Jul 30, 2021

[skip-ci][RNTupleDS] Test reading of RVecs #6361

Closed

[ntuple] Add 'std::' qualifier to int32_t, uint32_t #8776

Merged

eguiraud force-pushed the rntuple-rvec branch from 8acc95e to 312a878 Compare July 30, 2021 15:58

eguiraud commented Sep 8, 2021

View reviewed changes

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

eguiraud force-pushed the rntuple-rvec branch from 47e2b3e to fedddf2 Compare October 7, 2021 16:01

eguiraud force-pushed the rntuple-rvec branch from fedddf2 to 8d2b81a Compare October 8, 2021 18:04

eguiraud mentioned this pull request Oct 11, 2021

[ntuple] Avoid double delete in RField<RVec<T>>::DestroyValue #9097

Merged

eguiraud force-pushed the rntuple-rvec branch from 8d2b81a to 8d3ef0a Compare October 11, 2021 17:12

eguiraud marked this pull request as ready for review October 11, 2021 17:13

eguiraud requested a review from pcanal as a code owner October 11, 2021 17:13

eguiraud force-pushed the rntuple-rvec branch from 3dd157c to 3a00877 Compare April 22, 2022 16:31

eguiraud requested a review from jblomer April 22, 2022 16:34

jblomer mentioned this pull request May 4, 2022

[ntuple] Fix moving of complex elements in collections #10520

Closed

jblomer requested changes May 15, 2022

View reviewed changes

tree/ntuple/v7/inc/ROOT/RNTupleModel.hxx Outdated Show resolved Hide resolved

eguiraud and others added 13 commits May 19, 2022 10:12

[ntuple] Add test for type-erased reads of RVecs

5e1b047

[ntuple] Add RRVecField, for type-erased I/O of RVec

1c9779c

[ntuple] Remove special-casing of RVec<bool>

b125c0e

With RVec's redesign, RVec<bool> now behaves consistently with all other RVec<T>'s (it does not inherit vector<bool>'s behavior).

[ntuple] Test reading back std::vectors as RVecs

3b62038

[ntuple] Add RPrintValueVisitor support for RVecs

4620d14

Plus tests for type-erased and non-type-erased value printing.

[ntuple] Evaluate RRVecField value size only once and cache it

78e3cce

[ntuple] Do not destroy RVec elements before reading in new ones

43b5f40

RNTuple does not explicitly destroy existing values when reading, it only overwrites the bytes.

[ntuple] Use constexpr where appropriate

9075e9a

Co-authored-by: Philippe Canal <[email protected]>

[ntuple] Also test RNTuple+empty RVec entries

307bb5c

[ntuple] Factor out type-erased retrieval of RVec data members

3d83676

[ntuple][NFC] clang-format fixes

f82d8bd

eguiraud force-pushed the rntuple-rvec branch from 3a00877 to 734109c Compare May 19, 2022 08:18

eguiraud requested a review from jblomer May 19, 2022 08:19

jblomer approved these changes May 19, 2022

View reviewed changes

eguiraud merged commit d262262 into root-project:master May 19, 2022

eguiraud deleted the rntuple-rvec branch May 19, 2022 22:10

Conversation

eguiraud commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

Uh oh!

phsft-bot commented Jul 29, 2021

Uh oh!

ghost commented Jul 29, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepCode's analysis on #47e2b3 found:

Top issues

👉 View analysis in DeepCode’s Dashboard | Configure the bot

👉 The DeepCode service and API will be deprecated in August, 2021. Here is the information how to migrate. Thank you for using DeepCode 🙏 ❤️ !

Uh oh!

phsft-bot commented Jul 29, 2021

Failing tests:

Uh oh!

jblomer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phsft-bot commented Jul 30, 2021

Uh oh!

phsft-bot commented Jul 30, 2021

Uh oh!

phsft-bot commented Jul 30, 2021

Failing tests:

Uh oh!

phsft-bot commented Jul 30, 2021

Uh oh!

phsft-bot commented Jul 30, 2021

Uh oh!

phsft-bot commented Jul 30, 2021

Uh oh!

phsft-bot commented Jul 30, 2021

Errors:

Failing tests:

Uh oh!

Uh oh!

phsft-bot commented Oct 7, 2021

Uh oh!

eguiraud commented Oct 7, 2021

Uh oh!

phsft-bot commented Oct 7, 2021

Warnings:

Uh oh!

phsft-bot commented Oct 7, 2021

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Oct 8, 2021

Uh oh!

phsft-bot commented Oct 8, 2021

Failing tests:

Uh oh!

phsft-bot commented Oct 11, 2021

Uh oh!

phsft-bot commented Apr 14, 2022

Errors:

Uh oh!

eguiraud commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phsft-bot commented Apr 22, 2022

Uh oh!

jblomer commented May 4, 2022

Uh oh!

eguiraud commented May 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jblomer left a comment

eguiraud commented Jul 29, 2021 •

edited

Loading

ghost commented Jul 29, 2021 •

edited by ghost

Loading

eguiraud commented Apr 22, 2022 •

edited

Loading

eguiraud commented May 4, 2022 •

edited

Loading