Teach TFileMerger to work with TFile* handles. #1073

vgvassilev · 2017-09-28T20:57:57Z

This patch allows TFileMerger to work with externally created TFile-s. Being
able to control the creation of the TFile objects give us a chance to use
in-memory files. This is very helpful in benchmarking when we want to simulate
fast disks or we just want to avoid disk wearout.

phsft-bot · 2017-09-28T20:58:17Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-28T21:26:49Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-28T22:01:05Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

phsft-bot · 2017-09-28T22:04:04Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

phsft-bot · 2017-09-28T22:06:22Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

phsft-bot · 2017-09-28T22:08:37Z

Build failed on mac1012/native.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

phsft-bot · 2017-09-28T22:13:07Z

Build failed on slc6/gcc62.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

pcanal · 2017-09-28T23:13:40Z

io/io/test/TFileMergerTests.cxx

+   ASSERT_TRUE(t != nullptr);
+
+   double d;
+   t->SetBranchAddress(name, &d);


Setting branch address to a stack address requires to either call t->ResetBranchAddresses(); or delete the TTree (otherwise it is technically in an invalid state).

Does that mean that file.Get(name) allocates an object and it is the user's responsibility to deallocate it? If yes, I assume I can transform it into a unique_ptr and would that work well with having a t->ResetBranchAddresses();?

Does that mean that file.Get(name) allocates an object and it is the user's responsibility to deallocate it?

In the case of a TTree, the point is an implicit shared pointer. You can delete it (and the TFile will be informed) and you can let the TFile delete it (but your are not directly inform when it does unless you are registered in the list of cleanups).

If yes, I assume I can transform it into a unique_ptr

Yes, you can as long as you make sure that the lifetime of the unique_ptr is less or equal to the litetime of the TFile.

Should be done.

pcanal · 2017-09-28T23:14:33Z

io/io/test/TFileMergerTests.cxx

+
+static void CreateATuple(TMemFile &file, const char *name, double value)
+{
+   auto mytree = new TTree(name, "A tree");


to be (guaranteed to be) consistent, you may want to add

mytree->SetDirectory(&file);

Hm... would that mean I will have to reset it in the end of that function?

Hm... would that mean I will have to reset it in the end of that function?

Why? Because "file" in all your use case is already the current directory, adding SetDirectory would be a nop however if for somewhere 'file' is not the current directory at the time CreateATuple then the behavior of the function would not be what you expects. (it would attach the TTree to another file and thus the file.Write at the end would not store the TTree at alll).

And actually that reminds me we have a new constructor to use instead of (new + SetDirectory):

auto mytree = new TTree(name, "A tree", &file);

pcanal · 2017-09-28T23:23:21Z

io/io/test/TFileMergerTests.cxx

+   CreateATuple(b, "b_tree", 2.);
+
+   TFileMerger merger;
+   auto output = new TMemFile("output.root", "CREATE");


Why not having output being a unique_ptr? (Maybe using std::make_unique :) )

IIRC, the TMemFile got deleted by the merger.

Yes, but then you do:

merger.OutputFile(std::unique_ptr<TFile>(output));

So I don't see the difference :) (and yes ownership needs to pass for this function to the TMemFile ....

phsft-bot · 2017-09-29T06:56:24Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-29T07:34:45Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

phsft-bot · 2017-09-29T07:50:15Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

phsft-bot · 2017-09-29T07:56:22Z

Build failed on mac1012/native.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

phsft-bot · 2017-09-29T08:14:26Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

phsft-bot · 2017-09-29T08:34:01Z

Build failed on slc6/gcc62.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

phsft-bot · 2017-09-29T16:20:46Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

vgvassilev

Comments should be addressed.

phsft-bot · 2017-09-29T16:38:30Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-09-29T17:16:04Z

Build failed on slc6/gcc49.
See console output.

Failing tests:

projectroot.tree.treeplayer.test.gtest_tree_treeplayer_test_dataframe_simple

phsft-bot · 2017-09-29T17:17:49Z

Build failed on centos7/gcc49.
See console output.

Failing tests:

phsft-bot · 2017-09-29T17:20:04Z

Build failed on mac1012/native.
See console output.

Warnings:

/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/R.h:40:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/R_ext/RS.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/Rinternals.h:60:11: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/library/Rcpp/include/Rcpp/r/headers.h:57:10: warning: non-portable path to file '<RVersion.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/R_ext/Visibility.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/R.h:40:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/R_ext/RS.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/Rinternals.h:60:11: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/library/Rcpp/include/Rcpp/r/headers.h:57:10: warning: non-portable path to file '<RVersion.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]
/cvmfs/sft.cern.ch/lcg/releases/R/3.2.5-c8cca/x86_64-mac1012-clang81-opt/lib/R/include/R_ext/Visibility.h:29:10: warning: non-portable path to file '<RConfig.h>'; specified path differs in case from file name on disk [-Wnonportable-include-path]

And 10 more

Failing tests:

phsft-bot · 2017-09-29T17:40:50Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

phsft-bot · 2017-09-29T18:19:31Z

Build failed on slc6/gcc62.
See console output.

Failing tests:

pcanal · 2017-10-02T20:02:40Z

looks relevant to me?

Maybe, maybe not. This is the list of cleanups.

Axel-Naumann · 2017-10-02T20:25:12Z

Well we need a stacktrace of the crash to correlate. E.g. the TMemFiles are generally subject to the list of cleanup thread issues (disclaimer: I didn't check the patch).

eguiraud · 2017-10-02T20:41:37Z

here is a stacktrace. I think an interesting piece of information would be the name of the gDirectory when ~TContext bombs and the name of the directory its destructor would like to set as gDir.

pcanal · 2017-10-02T20:46:09Z

#8  <signal handler called>
#9  0x00007f089b3f153c in TDirectory::UnregisterContext(TDirectory::TContext*) () at ../root/core/base/src/TDirectory.cxx:1247
#10 0x00000000004ea2ee in TDirectory::TContext::~TContext() () at include/TDirectory.h:79

This usually happens when a thread has/had a gDirectory pointing to a file/directory and that this directory is deleted by another thread.

vgvassilev · 2017-10-02T20:51:38Z

Is there a way to understand if those race conditions pre-existing to this patch?

Axel-Naumann · 2017-10-02T21:13:01Z

The issue being pre-existing is getting less and less likely, given the info here... But I think Danilo and Enrico know this kind of issue from previous code versions; I bet you can solve this with their help!

eguiraud · 2017-10-02T21:33:26Z

yeah i'm here to help. in addition to the suggestions above, since you said that the crash disappears if you run the problematic test in isolation, it might be interesting to check whether the different threads have a gDirectory in a dirty state after the execution of the previous test.
Those invalid gDirs might be picked up by the TContexts in the test that crashes.

phsft-bot · 2017-10-04T20:49:12Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

This patch allows TFileMerger to work with externally created TFile-s. Being able to control the creation of the TFile objects give us a chance to use in-memory files. This is very helpful in benchmarking when we want to simulate fast disks or we just want to avoid disk wearout.

phsft-bot · 2017-10-04T21:02:36Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-10-04T21:59:45Z

Build failed on slc6/gcc62.
See console output.

Errors:

ERROR: Timeout after 10 minutes
ERROR: Error fetching remote repo 'origin'
stderr: error: RPC failed; result=18, HTTP code = 200
ERROR: Error fetching remote repo 'origin'

vgvassilev · 2017-10-05T06:41:47Z

@phsft-bot build!

Seems like the failure is infrastructure but let's retrigger.

phsft-bot · 2017-10-05T06:42:04Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

vgvassilev · 2017-10-05T08:16:19Z

I found the race condition, it was in the way we created the output TFile. It had to have TContext RAII object protecting the gDirectory.

@dpiparo shall I dismiss your request for changes as they are already in?

dpiparo · 2017-10-05T08:27:42Z

@vgvassilev if the changes are addressed, for sure. Thanks!

The requests for changes were addressed.

amadio · 2017-10-05T08:41:30Z

io/io/inc/ROOT/TBufferMerger.hxx

   std::function<void(void)> fCallback;                          //< Callback for when data is removed from queue

-   ClassDef(TBufferMerger, 0);
+   ClassDef(TBufferMerger, 1);


Why change this? This only matters for classes that are streamed. This should have remained as 0.

IIRC, when you have a ClassDef and you change data members you also should bump the ClassDef version.

IIRC, when you have a ClassDef and you change data members you also should bump the ClassDef version.

only when you change a persistent member :) ... and version == 0 means that all data members are implicitly marked transient.

Shall I revert it back to 0?

Ok, done. I don't understand how is that a non persistent data member but you are the expert.

pcanal · 2017-10-05T13:08:39Z

Ok, done. I don't understand how is that a non persistent data member but you are the expert.

Humm ... when the class version is set to zero then all the data member of the class are consider transient ... i.e. non-persistent .... When you increase to the version to 1, this turned implicitly all the data member to become persistent.

eguiraud · 2017-10-05T13:10:27Z

...by the way, why does TFileMerger need a `ClassDef` in the first place?

…

On Thu, Oct 5, 2017 at 3:08 PM, Philippe Canal ***@***.***> wrote: Ok, done. I don't understand how is that a non persistent data member but you are the expert. Humm ... when the class version is set to zero then *all* the data member of the class are consider transient ... i.e. non-persistent .... When you increase to the version to 1, this turned implicitly all the data member to become persistent. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1073 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKfU-nEGiaaVjwRDi9gIw3FcpqW7Ep8Cks5spNTZgaJpZM4Pn4OQ> .

pcanal · 2017-10-05T13:12:11Z

Any class that inherits from TObject must have a ClassDef otherwise some the routines are inconsistent (like IsA() ).

vgvassilev requested review from pcanal and peremato as code owners September 28, 2017 20:57

vgvassilev requested a review from amadio September 28, 2017 20:59

vgvassilev force-pushed the UseMemFiles branch from ca32b2e to 97a7c45 Compare September 28, 2017 21:26

pcanal reviewed Sep 28, 2017

View reviewed changes

vgvassilev force-pushed the UseMemFiles branch from 1d22da1 to 3a74914 Compare September 29, 2017 16:20

vgvassilev commented Sep 29, 2017

View reviewed changes

vgvassilev force-pushed the UseMemFiles branch from 3a74914 to d56481c Compare September 29, 2017 16:38

amadio assigned vgvassilev Oct 3, 2017

vgvassilev force-pushed the UseMemFiles branch from 440caba to c4ed4b9 Compare October 4, 2017 20:48

vgvassilev added 4 commits October 4, 2017 23:01

Report if the output file is writeable.

9257fff

Remove the comment. Now we SetDirectory(&file) which resolves the issue.

9be0fd1

Pin the symbol in libCore.

e8a02a7

vgvassilev force-pushed the UseMemFiles branch from c4ed4b9 to e8a02a7 Compare October 4, 2017 21:02

vgvassilev merged commit c409c25 into root-project:master Oct 5, 2017

vgvassilev deleted the UseMemFiles branch October 5, 2017 08:29

amadio reviewed Oct 5, 2017

View reviewed changes

phsft-bot mentioned this pull request May 17, 2023

[TCling] C++-use string,vector instead of std. #11027

Draft

phsft-bot mentioned this pull request Sep 13, 2023

[tmva][pymva]Various fixes in pymva #13634

Merged

Teach TFileMerger to work with TFile* handles. #1073

Teach TFileMerger to work with TFile* handles. #1073

Uh oh!

Conversation

vgvassilev commented Sep 28, 2017

Uh oh!

phsft-bot commented Sep 28, 2017

Uh oh!

phsft-bot commented Sep 28, 2017

Uh oh!

phsft-bot commented Sep 28, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 28, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 28, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 28, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 28, 2017

Failing tests:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Sep 29, 2017

Uh oh!

phsft-bot commented Sep 29, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 29, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 29, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 29, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 29, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 29, 2017

Uh oh!

vgvassilev left a comment

Choose a reason for hiding this comment

Uh oh!

phsft-bot commented Sep 29, 2017

Uh oh!

phsft-bot commented Sep 29, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 29, 2017

Failing tests:

Uh oh!

phsft-bot commented Sep 29, 2017

Warnings:

Failing tests: