[hist] Implement initial RHistEngine

[hahnjo]

It combines an RAxes object and storage of bin contents.

[github-actions]

Test Results

    20 files      20 suites   3d 17h 24m 11s ⏱️
 3 657 tests  3 656 ✅ 0 💤 1 ❌
71 458 runs  71 457 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 8928a0d.

♻️ This comment has been updated with latest results.

[hageboeck]

Looks good! The below comments are mostly for the documentation or usability.

[hageboeck]

Could one maybe format "<sizeof...(A)> vs <GetNDimensions>" into the string to make exception::what() more insightful, and also add the function name?

[hahnjo]

I was worried that dynamic exception messages with std::string could have a performance hit, and it seems to be the case when built with Clang:

RHistEngine_int_Regular1/Fill/4096       6645 ns         6637 ns       106185
RHistEngine_int_Regular2/Fill/4096      25946 ns        25915 ns        26760

goes to

RHistEngine_int_Regular1/Fill/4096      34293 ns        34253 ns        20438
RHistEngine_int_Regular2/Fill/4096      35925 ns        35883 ns        19754

GCC seems to be unaffected, maybe because of different code layout? I would like to investigate; if possible without performance hit, we can later on implement it consistently also for the exceptions in RAxes.

[hahnjo]

Right, so with dynamic std::string concatenation neither Fill() nore ComputeGlobalIndex() are inlined anymore, which explains the performance penalty. It's possible to "fix" by making sure to outline the construction of the std::string into a separate function:

std::string InvalidNumberOfArguments(std::size_t N) const
{   
   std::string msg = "invalid number of arguments to Fill: ";
   msg += std::to_string(N) + " for " + std::to_string(GetNDimensions()) + " dimensions";
   return msg;
}

throw std::invalid_argument(InvalidNumberOfArguments(sizeof...(A)));

This gives:

RHistEngine_int_Regular1/Fill/4096       6640 ns         6632 ns       106346
RHistEngine_int_Regular2/Fill/4096      25708 ns        25678 ns        27067

However I fear this depends on the compiler (version) and might change at any point. I think I would prefer to keep constant exception message strings for performance critical functions.

Fun fact, I first tried to outline ThrowInvalidNumberOfArguments:

RHistEngine_int_Regular1/Fill/4096      18731 ns        18709 ns        37460
RHistEngine_int_Regular2/Fill/4096      30178 ns        30143 ns        23226

That's worse because the compiler cannot see throw in that case; adding [[unlikely]] brings it further down to:

RHistEngine_int_Regular1/Fill/4096      11911 ns        11897 ns        61084
RHistEngine_int_Regular2/Fill/4096      21541 ns        21516 ns        32604

[hageboeck]

OK, let's leave it like that.

[hageboeck]

Maybe one could make this a bit easier to trace by adding "RHistEngine::GetBinContent(): bin not found"?

[hahnjo]

So far the other exception messages don't prefix the function name. I would argue that the stack trace should be used to find where the exception was thrown...