Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fixups from code review.
  • Loading branch information
bbockelm committed Nov 4, 2016
commit e14e674236f67a021de4252c0059ee56a3cd6c70
15 changes: 14 additions & 1 deletion test/MainEvent.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,12 @@ int main(int argc, char **argv)
Int_t branchStyle = 1; //new style by default
if (split < 0) {branchStyle = 0; split = -1-split;}

ROOT::EnableImplicitMT(4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be optional (so we need to add a command line argument) and default to not be enabled (as is the test now complain if ROOT was not built with IMT enabled).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. Those were test modifications to MainEvent.cxx that I did not intend to commit. Will remove them all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree ;). I think it is a useful new testing swtich ... it just need to be made conditional. thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer that it is automatically enabled or add it as a command line flag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a command line flag so that the default stay as is and one can test with and without it with the same build.


TFile *hfile;
TTree *tree;
Event *event = 0;
Event *event2 = 0;

// Fill event, header and tracks with some random numbers
// Create a timer object to benchmark this loop
Expand Down Expand Up @@ -195,6 +198,7 @@ int main(int argc, char **argv)
} else
hfile = new TFile("Event.root","RECREATE","TTree benchmark ROOT file");
hfile->SetCompressionLevel(comp);
//hfile->SetCompressionAlgorithm(2);

// Create histogram to show write_time in function of time
Float_t curtime = -0.5;
Expand All @@ -213,9 +217,16 @@ int main(int argc, char **argv)
bufsize = 64000;
if (split) bufsize /= 4;
event = new Event(); // By setting the value, we own the pointer and must delete it.
event2 = new Event(); // By setting the value, we own the pointer and must delete it.
TTree::SetBranchStyle(branchStyle);
TBranch *branch = tree->Branch("event", &event, bufsize,split);
TBranch *branch2 = tree->Branch("event2", &event, bufsize,split);
TBranch *branch3 = tree->Branch("event3", &event2, bufsize,split);
TBranch *branch4 = tree->Branch("event4", &event2, bufsize,split);
branch->SetAutoDelete(kFALSE);
branch2->SetAutoDelete(kFALSE);
branch3->SetAutoDelete(kFALSE);
branch4->SetAutoDelete(kFALSE);
if(split >= 0 && branchStyle) tree->BranchRef();
Float_t ptmin = 1;

Expand All @@ -230,6 +241,7 @@ int main(int argc, char **argv)
}

event->Build(ev, arg5, ptmin);
event2->Build(ev, arg5, ptmin);

if (write) nb += tree->Fill(); //fill the tree

Expand All @@ -242,7 +254,8 @@ int main(int argc, char **argv)
}
}
// We own the event (since we set the branch address explicitly), we need to delete it.
delete event; event = 0;
// delete event; event = 0;
// delete event2; event2 = 0;

// Stop timer and print results
timer.Stop();
Expand Down
4 changes: 2 additions & 2 deletions tree/tree/inc/TTree.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,11 @@ class TTree : public TNamed, public TAttLine, public TAttFill, public TAttMarker
TTree(const TTree& tt); // not implemented
TTree& operator=(const TTree& tt); // not implemented

#ifdef R__USE_IMT
// For simplicity, although fIMTFlush is always disabled in non-IMT builds, we don't #ifdef it out.
mutable Bool_t fIMTFlush{false}; ///<! True if we are doing a multithreaded flush.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd to see a mutable non-atomic. Is that really the intention? (i.e. is it really never possible to modify/read it in parallel?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the intention. Well, the real problem here is that FlushBaskets is declared const: the mutable here is working around that.

The fIMTFlush is only called from within FlushBaskets, which is not allowed to be called from multiple threads. It's done outside the parallel portion, meaning there must be a synchronization after it is mutated.

I wanted to avoid making it atomic to fulfill the request that there is no performance penalty when ROOT is used in non-IMT mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, a comment here or at the use place along the line of this answer ought to be added.

mutable std::atomic<Long64_t> fIMTTotBytes; ///<! Total bytes for the IMT flush baskets
mutable std::atomic<Long64_t> fIMTZipBytes; ///<! Zip bytes for the IMT flush baskets.
#endif

void InitializeSortedBranches();
void SortBranchesByTime();

Expand Down
10 changes: 9 additions & 1 deletion tree/tree/src/TBasket.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -932,6 +932,12 @@ Int_t TBasket::WriteBuffer()
return -1;
}
fMotherDir = file; // fBranch->GetDirectory();

// This mutex prevents multiple TBasket::WriteBuffer invocations from interacting
// with the underlying TFile at once - TFile is assumed to *not* be thread-safe.
//
// The only parallelism we'd like to exploit (right now!) is the compression
// step - everything else should be serialized at the TFile level.
#ifdef R__USE_IMT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the slightly unusual use pattern (take the lock then explicit unlock then lock). Can you add comment on what this is propecting (and why it will not provoke a dead lock).

std::unique_lock<std::mutex> sentry(file->fWriteMutex);
#endif // R__USE_IMT
Expand Down Expand Up @@ -1000,7 +1006,9 @@ Int_t TBasket::WriteBuffer()
for (Int_t i = 0; i < nbuffers; ++i) {
if (i == nbuffers - 1) bufmax = fObjlen - nzip;
else bufmax = kMAXZIPBUF;
//compress the buffer
// Compress the buffer. Note that we allow multiple TBasket compressions to occur at once
// for a given TFile: that's because the compression buffer when we use IMT is no longer
// shared amongst several threads.
#ifdef R__USE_IMT
sentry.unlock();
#endif // R__USE_IMT
Expand Down
3 changes: 3 additions & 0 deletions tree/tree/src/TTree.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -4792,6 +4792,8 @@ Int_t TTree::Fit(const char* funcname, const char* varexp, const char* selection
return -1;
}

namespace {

struct BoolRAIIToggle {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider putting this class in the anonymous namespace.


Bool_t &m_val;
Expand All @@ -4801,6 +4803,7 @@ BoolRAIIToggle(Bool_t &val) : m_val(val) { m_val = true; }
~BoolRAIIToggle() { m_val = false; }
};

}

////////////////////////////////////////////////////////////////////////////////
/// Write to disk all the basket that have not yet been individually written.
Expand Down