Skip to content

Form fix ttree write threading #476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knoepfel merged 4 commits into from
Mar 31, 2026
Merged

Form fix ttree write threading #476

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f49029
Proposed fix for weird threading problems with TTree getting saved to
aolivier23 Mar 30, 2026
df3b5f6
Removed cout statements that we were using to demonstrate that this
aolivier23 Mar 31, 2026
c5b786c
Merge branch 'main' into form_fix_ttree_write_threading
aolivier23 Mar 31, 2026
8be135e
Removed comment about changes in this PR.
aolivier23 Mar 31, 2026
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
4 changes: 2 additions & 2 deletions form/root_storage/root_ttree_write_container.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ ROOT_TTree_Write_ContainerImp::ROOT_TTree_Write_ContainerImp(std::string const&
ROOT_TTree_Write_ContainerImp::~ROOT_TTree_Write_ContainerImp()
{
if (m_tree != nullptr) {
m_tree->Write();
m_tree->GetDirectory()->WriteTObject(m_tree);
Comment thread
aolivier23 marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@pcanal pcanal Mar 31, 2026

Choose a reason for hiding this comment

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

Suggested change
m_tree->GetDirectory()->WriteTObject(m_tree);
m_tree->Write();

It should already be doing what (and a few more thing) the PR does. (On the other the SetDirectory is probably necessary to ensure that the TTree ends up in the right file).

delete m_tree;
}
}
Expand All @@ -42,8 +42,8 @@ void ROOT_TTree_Write_ContainerImp::setupWrite(std::type_info const& /* type*/)
m_tree = m_tfile->Get<TTree>(name().c_str());
}
if (m_tree == nullptr) {
TDirectory::TContext context(m_tfile.get());
Comment thread
aolivier23 marked this conversation as resolved.
m_tree = new TTree(name().c_str(), name().c_str());
m_tree->SetDirectory(m_tfile.get());
Comment on lines 45 to +46
Copy link
Copy Markdown
Contributor

@pcanal pcanal Mar 31, 2026

Choose a reason for hiding this comment

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

The fix is unlikely to solve the underlying problem.

If things are set properly (i.e. Thread Safety is enabled), the 2 codes have the same effective behavior.

TContext set gDirectory to m_tfile. If Thread Safety is enabled gDirectory is thread local and thus can not be influence by other threads.

If the new code is indeed more robust than the older code, it would (most likely) mean that Thread Safety is not enabled ... and if it not enabled all of ROOT/Core/Meta is not thread safe and will fail in many circumstances.

If Thread Safely is already enabled, then we have a deeper mystery to investigate.

Copy link
Copy Markdown
Contributor Author

@aolivier23 aolivier23 Apr 1, 2026

Choose a reason for hiding this comment

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

I think that must be it. Thread safety is probably not enabled. FORM doesn't do it to my knowledge, and I think we're the only component using ROOT. Thank you!

Copy link
Copy Markdown
Contributor

@pcanal pcanal Apr 1, 2026

Choose a reason for hiding this comment

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

Note that there is no harm in enabling it multiple time, so indeed FORM must enabled it explicitly.

}
if (m_tree == nullptr) {
throw std::runtime_error("ROOT_TTree_Write_ContainerImp::setupWrite no tree created");
Expand Down
Loading