Skip to content

[io] don't miss writing a histogram that is only in a last file with option -n 2 #18679

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[io] don't miss writing a histogram that is only in a last file with option -n 2 #18679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5111f69
[io] don't miss writing a histogram that is only in a last file with …
ferdymercury May 9, 2025
cb44a2f
[test][io] add merge test for single file hist with -n 2
ferdymercury May 9, 2025
6a1e569
[io][test] remove stray comment
ferdymercury May 12, 2025
e5d0915
[io][nfc] add a print for debugging/optimization purposes
ferdymercury May 14, 2025
f312f3b
[nfc] debug1
ferdymercury May 14, 2025
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
6 changes: 4 additions & 2 deletions io/io/src/TFileMerger.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,7 +542,8 @@ Bool_t TFileMerger::MergeOne(TDirectory *target, TList *sourcelist, Int_t type,
keyname, keytitle);
return kTRUE;
}
Bool_t canBeFound = (type & kIncremental) && (current_sourcedir->GetList()->FindObject(keyname) != nullptr);
Bool_t canBeFound = (type & kIncremental) && (current_sourcedir->GetList()->FindObject(keyname) != nullptr) &&
Copy link
Member

Choose a reason for hiding this comment

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

Why is it && and not || ?

Copy link
Member

Choose a reason for hiding this comment

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

Or more exactly, I don't understand yet why the fact the histogram can be found means that it is not written in the end ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Check out:

https://github.com/ferdymercury/root/blob/1531153ea11a7b54f4eb3c170bbd28e9bc46447f/io/io/src/TFileMerger.cxx#L808-L817

so if canBeFound is true, there is an optimization that spare some write cycles.

We use && to avoid it being true, ie to force writing to file. Using || would go in the different direction.

Do you want me to rename canBeFound to skipPartialWriting ?

Copy link
Member

Choose a reason for hiding this comment

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

Do you want me to rename canBeFound to skipPartialWriting ?

Not yet. I am still confused.

The original canBeFound meant (in the context of incremental merge) 'histogram can be found in the source' while the new version is histogram can be found in the source and in the target.

The optimization is indeed 'skip partial writing if we can found the histogram again'.

So I don't understand (yet) the semantic of the change. ie. Why is the new criteria the right choice? Is the new criteria instead 'just' making canBeFound always false?

Another avenue of inquiry is 'the original code assume that if canBeFound is true then there will be another change to write the histogram. Why is it no true anymore (it does not seem to be realted to 'can not be found in target')? Is there other variation of the example that also fails (how does the -n X value relates to the number of files in the input list and how many are 'missing' the histograms).

Related: is it possible that the alternative if that at the refresh boundary there needs to be a flush/write as if we were at the end?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review. I am not sure about these questions; I just followed jblomer's suggestion. My (limited) understanding is that this change just forces an extra partial write the first time that a new histogram appear in any file. So it does not really harm, but is suboptimal since, if all files have exactly the same histograms, then we could have waited until the end. But it makes it work if there are some files with and some without, independently of the chosen N.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but when going through the first file, it would not be (yet) in both the input and output, so it would be (spuriously) written, or am I still missing something?

Copy link
Collaborator Author

@ferdymercury ferdymercury Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm well, this is the result I get in debug mode:

Info in TFileMerger::MergeOne: Writing partial result of h1 into target
Info in TFileMerger::MergeOne: Writing partial result of h3 into target
Info in TFileMerger::MergeOne: Writing partial result of h2 into target

So... are you referring to the second line Writing partial result of h3 into target when you say spurious write?

Copy link
Member

Choose a reason for hiding this comment

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

I think so. Since h3 is in every file, shouldn't it not need any partial write?

Copy link
Collaborator Author

@ferdymercury ferdymercury Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Behaviour prior to this PR:

Info in <TFileMerger::MergeOne>: Writing partial result of h1 into target
Info in <TFileMerger::MergeOne>: Writing partial result of h3 into target

This happens because for the first file, (type & kIncremental) is false.

Behavior after this PR:

Info in <TFileMerger::MergeOne>: Writing partial result of h1 into target
Info in <TFileMerger::MergeOne>: Writing partial result of h3 into target
Info in <TFileMerger::MergeOne>: Writing partial result of h2 into target

So, this PR is only improving things, right?

If I understand you correctly, on top of this improvement, you would like to additionally see h3 not partially written at all:

Info in <TFileMerger::MergeOne>: Writing partial result of h1 into target
Info in <TFileMerger::MergeOne>: Writing partial result of h2 into target

to be even more performant, but what you suggest would require to traverse all the files in advance to know which key should be not partially written at all if it's in all files. I think that would be less efficient than one extra partial write in the first occurrence of a key throughout the list of files; right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, let's do a more elaborate example:

auto filename0 = "f0_9022.root";
auto filename1 = "f1_9022.root";
auto filename2 = "f2_9022.root";
auto outname = "file9022mergeroutput.root";
TFile f0(filename0, "RECREATE");
TH1F h0("h0", "h0", 1, 0, 1);
h0.Write();
f0.Close();
TFile f1(filename1, "RECREATE");
TH1F h("h1", "h1", 1, 0, 1);
h.Write();
TH1F h3("h3", "h3", 1, 0, 1);
h3.Write();
f1.Close();
TFile f2(filename2, "RECREATE");
TH1F h2("h2", "h2", 1, 0, 1);
h2.Write();
h3.Write();
f2.Close();
TFileMerger filemerger{false, false};
filemerger.SetMaxOpenedFiles(2);
filemerger.OutputFile(std::unique_ptr<TFile>{TFile::Open(outname, "RECREATE")});
filemerger.AddFile(filename0);
filemerger.AddFile(filename1);
filemerger.AddFile(filename2);
gDebug = 1;
filemerger.Merge();

So before the PR we get:

Info in <TFileMerger::MergeOne>: Writing partial result of h0 into target

(because for first type, type&incremental is false).

And the output file only contains histogram h0.

After the PR:

Info in <TFileMerger::MergeOne>: Writing partial result of h0 into target
Info in <TFileMerger::MergeOne>: Writing partial result of h1 into target
Info in <TFileMerger::MergeOne>: Writing partial result of h3 into target
Info in <TH1Merger::ExamineHistogram>: Examine histogram h3 - labels 1 - same limits 1 - axis found 1
Info in <TH1Merger::ExamineHistogram>: Examine histogram h3 - labels 1 - same limits 1 - axis found 1
Info in <TH1Merger::LabelMerge>: Merging histogram h3 into h3
Info in <TFileMerger::MergeOne>: Writing partial result of h2 into target

And the output file contains all histograms.

So the question is: why do you think it is a problem to have a first write whenever it appears? It's the only way to make it work so that the next appearance can call Merge. It's true that with a higher value of SetMaxOpenedFiles and with all files containing exactly the same hists, it's less optimum with this change, but still, this version is safer I guess?...

(target->GetList()->FindObject(keyname) != nullptr);

// if (cl->IsTObject())
// obj->ResetBit(kMustCleanup);
Expand Down Expand Up @@ -806,7 +807,8 @@ Bool_t TFileMerger::MergeOne(TDirectory *target, TList *sourcelist, Int_t type,
delete ndir;
}
} else if (!canBeFound) { // Don't write the partial result for TTree and TH1

if (gDebug > 0)
Info("MergeOne", "Writing partial result of %s into target", oldkeyname.Data());
if (!canBeMerged) {
TIter peeknextkey(nextkey);
status = WriteCycleInOrder(oldkeyname, nextkey, peeknextkey, target) && status;
Expand Down
36 changes: 36 additions & 0 deletions io/io/test/TFileMergerTests.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,3 +219,39 @@ TEST(TFileMerger, ChangeFile)
gSystem->Unlink("file6640mergerinput_2.root");
gSystem->Unlink("file6640mergeroutput.root");
}

// https://github.com/root-project/root/issues/9022
TEST(TFileMerger, SingleHistFile)
{
auto filename1 = "f1_9022.root";
auto filename2 = "f2_9022.root";
auto outname = "file9022mergeroutput.root";
{
TFile f1(filename1, "RECREATE");
TH1F h("h1", "h1", 1, 0, 1);
h.Write();
f1.Close();
TFile f2(filename2, "RECREATE");
TH1F h2("h2", "h2", 1, 0, 1);
h2.Write();
f2.Close();
}
{
TFileMerger filemerger{false, false};
filemerger.SetMaxOpenedFiles(2);
filemerger.OutputFile(std::unique_ptr<TFile>{TFile::Open(outname, "RECREATE")});

filemerger.AddFile(filename1);
filemerger.AddFile(filename2);

filemerger.Merge();
}
{
TFile file(outname, "READ");
EXPECT_NE(file.Get<TH1>("h1"), nullptr);
EXPECT_NE(file.Get<TH1>("h2"), nullptr);
}
gSystem->Unlink(filename1);
gSystem->Unlink(filename2);
gSystem->Unlink(outname);
}
Loading