Merge custom memory pool changes

As requested by @rmatev on !2270 (merged).

The complete set of MRs that relate to custom memory pools are:

• gaudi/Gaudi!1026 (merged) (no dependencies; some duplication can be removed once gaudi/Gaudi!1026 (merged) and !2270 (merged) are both merged)
• !2270 (merged) (adds infrastructure, no dependencies)
  • !2338 (merged) (depends on !2270 (merged), may need to wait for a ROOT patch)
  • !2339 (merged) (depends on !2270 (merged))
    • Rec!1892 (merged) (depends on !2339 (merged))
  • !2340 (merged) (depends on !2270 (merged))
    • Rec!1893 (merged) (depends on !2340 (merged))
  • !2341 (merged) and Rec!1895 (merged) (depend on each other and on !2270 (merged))
  • !2342 (merged) (depends on !2270 (merged))
  • !2343 (merged) (depends on !2270 (merged))
    • Rec!1896 (merged) (depends on !2343 (merged))
      • Phys!650 (merged) (depends on Rec!1896 (merged))

I tried to tabulate the actual dependencies that need to be respected, but I think this makes it look much more complicated than we really need to worry about. I would suggest:

• Ignore gaudi/Gaudi!1026 (merged), which can be handled asynchronously
• Merge !2270 (merged) "now"
• Group the rest (excluding !2338 (merged)) as Enable event-local memory pool for LHCb::Pr::Ve... (!2339 - merged)Rec!1892 (merged), Enable event-local memory pool for LHCb::Pr::{U... (!2340 - merged)Rec!1893 (merged), Make classes relevant to the SciFi/forward trac... (!2341 - merged)Rec!1895 (merged), !2342 (merged), Enable event-local memory pool for vertex and s... (!2343 - merged)Enable event-local memory pool in PV reconstruc... (Rec!1896 - merged)Phys!650 (merged). It should be fine to do anything on the spectrum from testing+merging each group separately to testing+merging all 5 groups in one go. This can just be decided based on their review/approval status at the relevant time.
• Merge !2338 (merged) "later" once the ROOT fix has been verified and propagated to LCG etc.

mentioned in merge request !2270 (merged)

this is extremely helpful, thank you!

assigned to @rcurrie and @rmatev

changed the description

@rcurrie could you please label with lhcb-head all MRs listed here (except gaudi/Gaudi and !2338 (merged))? The slot is clean from other MRs so it will allow us to test this full suite of changes, including for throughput. 🤞 for merging tomorrow

I realise that I forgot one other dependency: Rec!1896 (merged) depends on gaudi/Gaudi!1031 (merged) (it's listed in the description of Rec!1896 (merged) but not in this issue).

oof, I missed that. Let's remove lhcb-head from Rec!1896 (merged) and !2343 (merged) and Phys!650 (merged) then, unless you can find a way to work around the Gaudi bug

@clemenci approved gaudi/Gaudi!1031 (merged) an hour ago, so it might be merged imminently(?) Otherwise then yes, I think we need to remove the label from those ones.

even then, merging in Rec would prevent us from releasing the stack until gaudi is released. We'll have to slightly delay merging this bit

Ah, right. I might be able to add a workaround this afternoon; give me a chance to get that working, otherwise we'll have to unlabel.

I unlabled already but I'm happy to put them back if you manage

labels are back 🎉

👍 -- for the record the workaround is Rec@d1e1cd83.

🤞 for tomorrow's lhcb-head

unassigned @rcurrie

unassigned @rmatev

assigned to @rjhunter

https://lhcb-nightlies.web.cern.ch/logs/tests/nightly/lhcb-head/2500/x86_64-centos7-gcc9-opt/Brunel/#failures_9 see the very bad failures in the test.

Looks like ROOT/cling can't cope with some of my new code..clang and gcc both think it's fine. I'll see what I can do.

Given that both seem to be linked to boost::container::small_vector in some way, it's conceivable that the change I mention in #73 (comment 3163067) will fix this. Let's see...

In my local tests, including that change, the Brunel tests pass. Let's try again tonight...

Nightlies still look 😢; more digging required.

At least I can reproduce some failures this morning.

I pushed 25e5092d, which sidesteps the problem. At least for me, locally, in the test I was using...

It's not a real fix, on some level this looks like a ROOT problem. If I have time I'll try to report it to them...

Also the Rec build on clang fails http://lhcb-nightlies.web.cern.ch/logs/build/nightly/lhcb-head/2500/x86_64-centos7-clang8-opt/Rec/#show_Phys_SelAlgorithms/Phys_SelAlgorithms_5

I saw this in the nightlies but I didn't see it locally + it's not obvious how the error relates to any of my changes. I can try to investigate...

I haven't really understood why this fails. The error says that the proper definition of std::less isn't available (only some Boost forward-declaration), but the errors comes from a file that #includes <functional>, which defines std::less...

Apparently adding #include <functional> before #include <boost/container/small_vector.hpp> in Kernel/LHCbKernel/Kernel/MonotonicArena.h avoids the problem. I can push that change to !2270 (merged)...

I pushed it to !2270 (merged).

This seems to have done the job.

Nice work!

The do0 build of LHCb also fails in what looks like a related way http://lhcb-nightlies.web.cern.ch/logs/build/nightly/lhcb-head/2500/x86_64-centos7-gcc9-do0/LHCb/#show_Kernel_SOAExtensions/Kernel_SOAExtensions_8

It's not obvious how this relates to my changes, but I can take a look..

I failed to reproduce this locally, but @clemenci managed to. Looks like a very odd and very specific compiler bug. I pushed 02fe3491 to !2342 (merged) (arbitrarily) as a guess that will probably fix tonight's nightlies...if it works we can deploy it less hackily...

That seems to have done the job. @rmatev do you prefer that change to be split into a separate MR? Presumably we would prefer to have the change in https://gitlab.cern.ch/LHCbOpt/SOAContainer instead of my fork. Not sure how long that will take.

I opened LHCbOpt/SOAContainer!35 (merged).

Nice work @olupton!

I personally would not call it a fix: it's a work around for a bug in gcc, which we should report.

I agree on having LHCbOpt/SOAContainer!35 (merged) merged, of course, but it's not a fix and if we do not get it fixed upstream it will hit us back in the future, and it will be very hard to link it back to this issue.

mentioned in issue Moore#131 (closed)

mentioned in merge request LHCbOpt/SOAContainer!35 (merged)

Okay, so I think that LHCb-head was broadly happy last night. There are a few failing tests in places, but I didn't spot anything that I think might relate to these MRs. However, I am far from an expert in what you are doing here, so probably we need someone else to take a look at the results before I recommend any merging.

I didn't see anything concerning. I looked into some of the unrelated-seeming failures, which led to Rec!1934 (merged).

https://lhcb-nightlies.web.cern.ch/nightly/lhcb-head/build/2504/

🎉 well done @olupton and everyone who helped testing and debugging obscure compiler issues 🎉

I think we can close this and follow up the only remaining MR on its own

closed

unassigned @rjhunter