Skip to content

[hist] Optimize bin label search - #23682

Open
dpiparo wants to merge 3 commits into
root-project:masterfrom
dpiparo:optimize_bin_label_search
Open

dpiparo wants to merge 3 commits into
root-project:masterfrom
dpiparo:optimize_bin_label_search

Conversation

@dpiparo

@dpiparo dpiparo commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

Before linear search, assume the bin labels are ordered by unique id.

Fixes #23681

En passant, also fixes #23716

@dpiparo
dpiparo requested a review from silverweed October 11, 2026 10:54
@dpiparo dpiparo self-assigned this Oct 11, 2026
@github-actions

github-actions Bot commented Oct 11, 2026 •

Copy link
Copy Markdown

Test Results

    24 files      24 suites   3d 23h 46m 22s ⏱️
 3 889 tests  3 888 ✅ 0 💤 1 ❌
83 246 runs  83 245 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 25978bc.

♻️ This comment has been updated with latest results.

@dpiparo
dpiparo force-pushed the optimize_bin_label_search branch from caebd84 to dbb2422 Compare October 11, 2026 18:57
@silverweed

Copy link
Copy Markdown
Contributor

This might help in the majority of cases (to be validated), but a way to be sure to fix the original issue is to pre-sort all labels once before the loop, to change the complexity from O(nbins^2) to O(nbins).
Something like:

std::vector<const TObjString *> sortedLabels(labelsX->GetEntries() + labelsY->GetEntries() + labelsZ->GetEntries());
for (const auto *label : TRangeStaticCast<const TObjString>(labelsX))
  sortedLabels[label->GetUniqueId()] = label;
for (const auto *label : TRangeStaticCast<const TObjString>(labelsY))
  sortedLabels[labelsX->GetEntries() + label->GetUniqueId()] = label;
// ...

// now we can call sortedLabels[binx] instead of hist->GetXaxis()->GetBinLabel(binx) in the loop

@dpiparo
dpiparo force-pushed the optimize_bin_label_search branch from dbb2422 to 8251189 Compare October 14, 2026 12:33

@silverweed silverweed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We're not really avoiding linear scaling (that's pretty much unavoidable given the situation), so the commit message should probably not mention it

}
// Cache bins in vectors
auto getLabelsCache = [](TAxis *axis) {
std::vector<const char *> labels;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could/should avoid 2 allocations by using a single vector for all 3 axes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At the cost of readability perhaps?

EXPECT_DOUBLE_EQ(hist.Interpolate(0.3, 19.9, 5.), 100. * 19 + 10000. * 4.5);
EXPECT_DOUBLE_EQ(hist.Interpolate(0.8, 19.9, 5.), 0.3 + 100. * 19 + 10000. * 4.5);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#include "TList.h"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thanks for the comment, and to debian13 that failed to compile!

Before linear search, assume the bin labels are ordered by unique id.
Speeds up merging, reported to be slow by root-project#23681
@dpiparo
dpiparo force-pushed the optimize_bin_label_search branch from 8251189 to 25978bc Compare October 14, 2026 20:13
@dpiparo dpiparo closed this Oct 15, 2026
@dpiparo dpiparo reopened this Oct 15, 2026
therefore speeding up identification of the bin label.

Fixes root-project#23681
@dpiparo
dpiparo force-pushed the optimize_bin_label_search branch from 25978bc to ee6847f Compare October 15, 2026 10:05
@dpiparo

dpiparo commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

We're not really avoiding linear scaling (that's pretty much unavoidable given the situation), so the commit message should probably not mention it

Good point, thanks. Done.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[hist] Merging 3D histograms with alphanumeric labels on the axes is broken Quadratic performance in TH1Merger::LabelMerge

3 participants