Skip to content

WIP: xAOD::EventFormat Creation Upgrade, master branch (2020.06.16.)

After @keli's very useful reminder a few days ago, I now tried to look into how to create the xAOD::EventFormat metadata for (D)xAOD files safely in multi-threaded jobs.

First off, while looking at the code and trying to figure out how to do all this, I had to realise that a unit test in xAODEventFormat has been laying dormant ever since our CMT->CMake transition. 😦 So I (re-)enabled it, and added an additional check to it which makes sure that the auto-generated copy constructor and operator of xAOD::EventFormat would behave as expected. It may be worth mentioning that it was this unit test that triggered my inquiry on atlas-sw-core about comparing lines with hexadecimal numbers in unit tests. (Ping @fwinkl. 😄)

As a reminder: Right now xAODMaker::EventFormatSvc operates completely on its own, making use of IncidentSvc to receive callbacks at the end of each event, and when the metadata would need to be written. Since IncidentSvc is not an AthenaMT friendly construct, I was trying to implement the code without relying on it at all.

To do this, I now ended up with 3 components. 😕

  • xAODMaker::EventFormatSvc became a thread-safe service that doesn't do anything by itself, it acts sort of as a store for the metadata during the job. It's a fair question though whether it is really still needed. But more about it in a bit.
  • xAODMaker::EventFormatStreamHelperTool is a tool that is supposed to be added as a "helper tool" (https://gitlab.cern.ch/atlas/athena/-/blob/master/Control/AthenaServices/src/AthenaOutputStream.cxx#L184) to every output stream that is writing an xAOD format file. Its only purpose is to tell xAODMaker::EventFormatSvc to collect all the metadata that it needs, from the StoreGateSvc instance that is being "written from".
  • xAODMaker::EventFormatMetaDataMakerTool is the "metadata tool" in this whole setup. It is notified by MetaDataSvc at the end of the job when metadata is supposed to be written to the metadata store. It grabs the metadata collected by xAODMaker::EventFormatSvc at that point, and records it into the metadata store.
    • Note that xAODMaker::EventFormatSvc returns a copy of the xAOD::EventFormat object that it created, so that there would definitely be no clash between a call coming from an xAODMaker::EventFormatStreamHelperTool instance, and one coming from xAODMaker::EventFormatMetaDataMakerTool. This is probably an unnecessary overhead, but a small one at that. (This is the reason why I added the additional tests in xAODEventFormat.)

Now... Much of this is probably a bit of over-engineering. But since in the future output streams may actually be writing events "somehow" at the same time, it seemed logical to set up a single point (xAODMaker::EventFormatSvc) that all the streams would be "talking to". (Through xAODMaker::EventFormatStreamHelperTool instances.) It is possible that one could get rid of xAODMaker::EventFormatSvc completely, and have the xAODMaker::EventFormatStreamHelperTool instances all interact with MetaDataStore directly. But I wasn't sure if that would be safe, or whether we would even want to make that thread-safe. (Multiple clients grabbing non-const pointers to an xAOD::EventFormat object at the same time.)

I still want to extend this MR a bit with some more thorough testing, but at this point I wanted to get some feedback. @mnowak, @gemmeren, @tsulaia, @ssnyder, what do you guys think about such a setup?

Note that I had to realise while writing my tests, that SG::ReadMetaHandle is not functional yet. I was hoping to use it in my xAODMakerTest::EventFormatPrinterAlg algorithm, but it didn't work. (And I didn't want to include the fix of that class in this MR.) The problem is that that handle at the moment always tries to retrieve a non-const pointer to an object held by MetaDataStore. While in my case I would need to retrieve a const pointer to an object in InputMetaDataStore. 🤔 I guess it's known that the handle is not functional yet since while a test algorithm for it exists (https://gitlab.cern.ch/atlas/athena/-/blob/master/Control/DataModelTest/DataModelTestDataCommon/src/MetaReaderAlg.h), it's not actually used by any test. Because I believe it would not work either...

So we'll need to fix up that code as well in the near future...

Edited by Attila Krasznahorkay

Merge request reports