From acbc8e2a3dd2ed8fb665fdfbfe106f1ecba5f70b Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Mon, 24 Jul 2023 16:18:05 +0200 Subject: [PATCH 01/10] Create conditions with a null payload for missing files or conditions --- Core/include/Core/ConditionIdentifier.h | 2 - Core/include/Core/ConditionsRepository.h | 22 +++- Core/src/ConditionsGitReader.cpp | 47 +++---- Core/src/ConditionsLoader.cpp | 153 +++++++++++++++++------ Core/src/ConditionsRepository.cpp | 11 +- Core/src/DetectorDataService.cpp | 13 +- 6 files changed, 175 insertions(+), 73 deletions(-) diff --git a/Core/include/Core/ConditionIdentifier.h b/Core/include/Core/ConditionIdentifier.h index dc5195de9..be653bc71 100644 --- a/Core/include/Core/ConditionIdentifier.h +++ b/Core/include/Core/ConditionIdentifier.h @@ -14,8 +14,6 @@ //========================================================================== #pragma once -#include "Core/ConditionsRepository.h" - #include "DD4hep/Conditions.h" #include "DD4hep/DetElement.h" #include "DDCond/ConditionsContent.h" diff --git a/Core/include/Core/ConditionsRepository.h b/Core/include/Core/ConditionsRepository.h index 19b00f8cf..97814a43e 100644 --- a/Core/include/Core/ConditionsRepository.h +++ b/Core/include/Core/ConditionsRepository.h @@ -18,9 +18,19 @@ #include "DDCond/ConditionsContent.h" +#include <boost/multi_index/hashed_index.hpp> +#include <boost/multi_index/key_extractors.hpp> +#include <boost/multi_index/ordered_index.hpp> +#include <boost/multi_index_container.hpp> + +namespace { + namespace bmi = boost::multi_index; +} + namespace LHCb::Detector { - class ConditionIdentifier; + dd4hep::Condition::key_type location_key( const ConditionIdentifier* entry ); + struct RequestSelector; /** @@ -32,7 +42,15 @@ namespace LHCb::Detector { friend struct RequestSelector; public: - std::map<dd4hep::Condition::key_type, ConditionIdentifier*> locations; + using Locations = boost::multi_index_container< + ConditionIdentifier*, + bmi::indexed_by< + bmi::ordered_unique< + bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &location_key>>, + bmi::hashed_non_unique<bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>, + bmi::hashed_unique< + bmi::member<ConditionIdentifier, dd4hep::Condition::key_type, &ConditionIdentifier::hash>>>>; + Locations locations; private: /// Default assignment operator diff --git a/Core/src/ConditionsGitReader.cpp b/Core/src/ConditionsGitReader.cpp index 27a5d0b93..d077f99ce 100644 --- a/Core/src/ConditionsGitReader.cpp +++ b/Core/src/ConditionsGitReader.cpp @@ -95,30 +95,33 @@ int LHCb::Detector::ConditionsGitReader::getObject( const std::string& system_id // Now loading the condition // m_condDB->logger()->level = GitCondDB::Logger::Level::Debug; - auto [data, iov] = m_condDB->get( {m_dbtag, path, time_point} ); - if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) { - if ( iov.since != time_point ) { - throw std::runtime_error( - fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) ); + try { + auto [data, iov] = m_condDB->get( {m_dbtag, path, time_point} ); + + if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) { + if ( iov.since != time_point ) { + throw std::runtime_error( + fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) ); + } + iov.until = iov.since + 1; } - iov.until = iov.since + 1; - } - buffer = std::move( data ); - // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other, - // and make sure we do not use the part of uint64_t that long does not cover - ct->valid_since = iov.since; // the IOV start is OK by construction - if ( iov.until == GitCondDB::CondDB::IOV::max() ) { - ct->valid_until = dd4hep::IOV::MAX_KEY; - } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) { - throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until, - dd4hep::IOV::MAX_KEY, m_dbtag, path ) ); - } else { - // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV, - // while DD4hep includes it so we have to map [100,200) -> [100,199] - ct->valid_until = iov.until - 1; - } - return 1; + buffer = std::move( data ); + // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other, + // and make sure we do not use the part of uint64_t that long does not cover + ct->valid_since = iov.since; // the IOV start is OK by construction + if ( iov.until == GitCondDB::CondDB::IOV::max() ) { + ct->valid_until = dd4hep::IOV::MAX_KEY; + } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) { + throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until, + dd4hep::IOV::MAX_KEY, m_dbtag, path ) ); + } else { + // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV, + // while DD4hep includes it so we have to map [100,200) -> [100,199] + ct->valid_until = iov.until - 1; + } + return 1; + } catch ( std::runtime_error const& e ) { return 0; } } /// Resolve a given URI to a string containing the data diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp index a82745164..a9dd636a4 100644 --- a/Core/src/ConditionsLoader.cpp +++ b/Core/src/ConditionsLoader.cpp @@ -17,6 +17,7 @@ #include "Core/ConditionsLoader.h" #include "Core/ConditionsReaderContext.h" #include "Core/ConditionsRepository.h" +#include "Core/Keys.h" // Other dd4hep includes #include "Core/UpgradeTags.h" @@ -31,6 +32,7 @@ #include "TTimeStamp.h" #include <Core/yaml_converters.h> +#include <boost/range/iterator_range_core.hpp> #include <exception> #include <set> #include <sstream> @@ -38,6 +40,24 @@ #include <utility> #include <vector> +namespace { + std::tuple<bool, YAML::Node> empty_condition( const LHCb::Detector::ConditionIdentifier* cond ) { + const std::array<dd4hep::Condition::key_type, 2> alignment_keys = {LHCb::Detector::Keys::deltaKey, + LHCb::Detector::Keys::alignmentKey}; + + dd4hep::ConditionKey::KeyMaker key( cond->hash ); + + bool is_alignment = std::any_of( alignment_keys.begin(), alignment_keys.end(), + [&key]( auto hash ) { return key.values.item_key == hash; } ); + + auto const* payload = is_alignment ? "NotFound: !alignment\n position: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n " + "rotation: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n" + : ""; + auto node = YAML::Load( payload ); + return {is_alignment, is_alignment ? node["NotFound"] : node}; + } +} // namespace + /// Standard constructor, initializes variables LHCb::Detector::ConditionsLoader::ConditionsLoader( dd4hep::Detector& description, dd4hep::cond::ConditionsManager mgr, const std::string& nam ) @@ -120,8 +140,50 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // as done so that we can skip the iteration. std::map<dd4hep::Condition::key_type, ToDoItem> todo_list{conditions_to_load.begin(), conditions_to_load.end()}; + auto block_register = [this]( dd4hep::IOV::Key iov_key, const std::vector<dd4hep::Condition>& entity_conditions ) { + dd4hep::cond::ConditionsPool* pool = manager().registerIOV( *m_iovType, iov_key ); + return manager().blockRegister( *pool, entity_conditions ); + }; + try { for ( auto [current_key, item] : todo_list ) { // O[N] + + auto register_condition = [&loaded, &todo_list, ¤t_key = current_key, + &item = item]( std::vector<dd4hep::Condition>& entity_conditions, + const ConditionIdentifier* cond_ident, std::string cond_name, + const YAML::Node& cond_data, std::string doc ) { + dd4hep::Condition c = LHCb::YAMLConverters::make_condition( cond_name, cond_data ); + if ( !c.isValid() ) { + // Will eventually have to raise an exception + dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Failed to load condition %s from %s", + cond_name.c_str(), doc.c_str() ); + return false; + } else { + const auto cond_key = cond_ident->hash; + + c->hash = cond_key; + // pass the empty condition to the caller + loaded[cond_key] = c; + + // record the condition for bulk registration to the manager + entity_conditions.push_back( c ); + + // if this is the condition we were looking for, + // flag the ToDoItem as done and move to the next condition in the YAML object + if ( cond_key == current_key ) { + item.done = true; + return false; + } + // otherwise see if we are supposed to load it, + // in which case the pending work item can be marked as already dine + if ( auto cond_itr = todo_list.find( cond_key ); cond_itr != todo_list.end() ) { + cond_itr->second.done = true; + return false; + } + return true; + } + }; + const auto cond_id = item.cond_id; // FIXME this early skip for conditions already loaded from a URL does not convince me if ( item.done ) { continue; } @@ -131,6 +193,8 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, continue; } + const auto* repo = cond_id->repository; + // set::insert returns a pair where the second tells if the insertion // took place (true) or the entry was already present (false) if ( !processed_urls.insert( cond_id->sys_id ).second ) { @@ -141,13 +205,13 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // ready, set, chrono started! TTimeStamp start; // Load from the git cond db here XXX - // FIXME: if the interface make sense, we should do something in case of failure + // If the file can't be loaded, create empty conditions for all + // of the conditions expected in this file if ( m_reader->load( cond_id->sys_id, &ctxt, buffer ) ) { auto ext = std::string_view{cond_id->sys_id}; ext.remove_prefix( ext.find_last_of( '.' ) ); if ( ext == ".yml" || ext == ".yaml" ) { dd4hep::ConditionKey::KeyMaker kmaker( cond_id->sys_id_hash, 0 ); - const auto* repo = cond_id->repository; // condition YAML files are expected to contain a mapping <name> -> <condition data> auto data = YAML::Load( ( m_useOverlay ) ? m_overlay.get_or_default( cond_id->sys_id, buffer ) : buffer ); @@ -164,65 +228,61 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // prepare the store for the conditions we will register to the conditions manager // ... it would be nice if we had an actual type here and not a handle - std::vector<dd4hep::Condition> entity_conditons; - entity_conditons.reserve( data.size() ); + std::vector<dd4hep::Condition> entity_conditions; + entity_conditions.reserve( data.size() ); // loop over all conditions in the YAML document for ( const auto& cond_data : data ) { const std::string cond_name = cond_data.first.as<std::string>(); // see if the condition is in the list of locations(?) + // Only the lower 32 bits of kmaker.hash have been set + // above, set the upper bits here kmaker.values.item_key = dd4hep::detail::hash32( cond_name ); - auto cond_ident = repo->locations.find( kmaker.hash ); // O[log(N)] + + auto cond_ident = repo->locations.find( kmaker.hash ); // O[log(N)] if ( cond_ident == repo->locations.end() ) { dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Got stray condition %s from %s", cond_name.c_str(), cond_id->sys_id.c_str() ); continue; } - dd4hep::Condition c = LHCb::YAMLConverters::make_condition( cond_name, cond_data.second ); - if ( !c.isValid() ) { - // Will eventually have to raise an exception - dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Failed to load condition %s from %s", - cond_name.c_str(), cond_id->sys_id.c_str() ); - continue; - } + bool stray = + register_condition( entity_conditions, *cond_ident, cond_name, cond_data.second, cond_id->sys_id ); + if ( !stray ) continue; - // FIXME: is this not the same as kmaker.hash? - const auto cond_key = cond_ident->second->hash; - c->hash = cond_key; - // pass the condition to the caller - loaded[cond_key] = c; - // record the condition for bulk registration to the manager - entity_conditons.push_back( c ); - // if this is the condition we were looking for, - // flag the ToDoItem as done and move to the next condition in the YAML object - if ( cond_key == current_key ) { - item.done = true; - continue; - } - // otherwise see if we are supposed to load it, - // in which case the pending work item can be marked as already dine - if ( auto cond_itr = todo_list.find( cond_key ); cond_itr != todo_list.end() ) { - cond_itr->second.done = true; - continue; - } // this warning means that we found in the YAML file a condition we were not requesting // FIXME I do not see why this is a problem dd4hep::printout( dd4hep::WARNING, "ConditionsLoader", "++ Got stray but valid condition %s from %s", cond_name.c_str(), cond_id->sys_id.c_str() ); } - // now that we converted the whole file we make a pool for the IOV of the YAML file - // and we register the conditions we found + // Check if there are conditions in this file that have been + // requested, but have not yet been loaded. If so, create + // empty conditions for those too. + for ( const auto* cond_ident : + boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) { + auto loaded_it = loaded.find( cond_ident->hash ); + if ( loaded_it == loaded.end() ) { + auto [is_alignment, cond] = empty_condition( cond_ident ); + register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id ); + dd4hep::printout( dd4hep::DEBUG, "ConditionsLoader", "++ Created empty%s condition %s in %s", + is_alignment ? " alignment" : "", cond_ident->object.c_str(), + cond_ident->sys_id.c_str() ); + } + } + + // now that we converted the whole file and created empty conditions for those that are missing, + // we make a pool for the IOV of the YAML file and we register the conditions; the IOV for missing + // conditions should be the same as for those that were found, as they may only appear at the end of + // the IOV // FIXME why one pool per YAML file? should it not be one pool for all conditions? (given an event time) dd4hep::IOV::Key iov_key( ctxt.valid_since, ctxt.valid_until ); // FIXME where does the IOVType comes from? why it is bound the the ConditionsLoader instance? - dd4hep::cond::ConditionsPool* pool = manager().registerIOV( *m_iovType, iov_key ); - size_t ret = manager().blockRegister( *pool, entity_conditons ); + size_t ret = block_register( iov_key, entity_conditions ); // FIXME it looks like blockRegister returns the number of conditions registered and we have to check // that it's what we expect, but clearly we do not know what to do with that information - if ( ret != entity_conditons.size() ) { + if ( ret != entity_conditions.size() ) { // Error! } } @@ -237,6 +297,27 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( ctxt.valid_since ).c_str(), valid_end.c_str(), stop.AsDouble() - start.AsDouble() ); loaded_len = loaded.size(); + } else { + // File containing Conditions not found, create empty + // conditions for all requested conditions that are in this + // file. + std::vector<dd4hep::Condition> entity_conditions; + entity_conditions.reserve( repo->locations.get<1>().count( cond_id->sys_id_hash ) ); + + for ( const auto* cond_ident : + boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) { + auto [is_alignment, cond] = empty_condition( cond_ident ); + register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id ); + } + + // If the file is not in the DB, it's never going to appear, + // so register empty conditions with an infinite IOV + dd4hep::IOV::Key iov_key( 0, std::numeric_limits<std::int64_t>::max() ); + block_register( iov_key, entity_conditions ); + + dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ Added %4ld null conds for %-48s to pool [%6s-%6s]", + loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( 0 ).c_str(), "inf " ); + loaded_len = loaded.size(); } if ( print_results ) { // for each requested key we actually load (not skipped) we print all loaded conditions diff --git a/Core/src/ConditionsRepository.cpp b/Core/src/ConditionsRepository.cpp index f5722b7cf..8a61ac149 100644 --- a/Core/src/ConditionsRepository.cpp +++ b/Core/src/ConditionsRepository.cpp @@ -18,11 +18,16 @@ //========================================================================== // Framework includes -#include "Core/ConditionIdentifier.h" +#include "Core/ConditionsRepository.h" #include "DDCond/ConditionsContent.h" #include <iostream> +dd4hep::Condition::key_type LHCb::Detector::location_key( const ConditionIdentifier* entry ) { + dd4hep::ConditionKey::KeyMaker loc( entry->sys_id_hash, entry->object_hash ); + return loc.hash; +} + // C/C++ include files template <> std::string dd4hep::cond::ConditionsContent::LoadInfo<LHCb::Detector::ConditionIdentifier>::toString() const { @@ -48,7 +53,7 @@ LHCb::Detector::ConditionsRepository::addLocation( dd4hep::DetElement de, dd4hep auto* info = new LoadInfo<ConditionIdentifier>( ident ); auto ret = addLocationInfo( km.hash, info ); dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash ); - locations.insert( std::make_pair( loc.hash, &info->info ) ); + locations.insert( &info->info ); return ret; } @@ -67,7 +72,7 @@ LHCb::Detector::ConditionsRepository::addGlobal( dd4hep::DetElement de, dd4hep:: auto* info = new LoadInfo<ConditionIdentifier>( ident ); auto ret = addLocationInfo( km.hash, info ); dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash ); - locations.insert( std::make_pair( loc.hash, &info->info ) ); + locations.insert( &info->info ); return ret; } diff --git a/Core/src/DetectorDataService.cpp b/Core/src/DetectorDataService.cpp index 6810a69f9..e03881d0b 100644 --- a/Core/src/DetectorDataService.cpp +++ b/Core/src/DetectorDataService.cpp @@ -182,11 +182,10 @@ void LHCb::Detector::DetectorDataService::initialize( const nlohmann::json& conf auto cond_key = LHCb::Detector::ConditionKey( m_description.detector( std::string{deName} ), std::string{condKey} ); // Check in the conditions mappings which file this corresponds to - if ( auto el = std::find_if( begin( m_all_conditions->locations ), end( m_all_conditions->locations ), - [cond_key]( const auto& item ) { return item.second->hash == cond_key; } ); - el != end( m_all_conditions->locations ) ) { + if ( auto el = m_all_conditions->locations.get<2>().find( cond_key ); + el != end( m_all_conditions->locations.get<2>() ) ) { overrides.emplace_back( - LHCb::Detector::ConditionOverrideEntry{el->second->sys_id, el->second->object, YAML::Load( o.second )} ); + LHCb::Detector::ConditionOverrideEntry{( *el )->sys_id, ( *el )->object, YAML::Load( o.second )} ); } else { throw std::invalid_argument{"cannot find " + std::string{condKey} + " in " + std::string{deName} + "'s ConditionsRepository"}; @@ -325,10 +324,8 @@ void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetEle void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetElement& de, const std::string& cond, const YAML::Node& data ) { auto key = LHCb::Detector::ConditionKey( de, cond ); - if ( auto el = std::find_if( begin( m_all_conditions->locations ), end( m_all_conditions->locations ), - [key]( const auto& item ) { return item.second->hash == key; } ); - el != end( m_all_conditions->locations ) ) { - update_condition( el->second->sys_id, el->second->object, data ); + if ( auto el = m_all_conditions->locations.get<2>().find( key ); el != end( m_all_conditions->locations.get<2>() ) ) { + update_condition( ( *el )->sys_id, ( *el )->object, data ); } else { throw std::invalid_argument{"cannot find " + cond + " in " + de.name() + "'s ConditionsRepository"}; } -- GitLab From 13849eda82cc4da367779966a485b9206a161279 Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Mon, 24 Jul 2023 16:18:16 +0200 Subject: [PATCH 02/10] Fix compiler warnings --- Core/tests/src/test_demuon.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Core/tests/src/test_demuon.cpp b/Core/tests/src/test_demuon.cpp index e39db717c..8c005d6d6 100644 --- a/Core/tests/src/test_demuon.cpp +++ b/Core/tests/src/test_demuon.cpp @@ -114,7 +114,7 @@ void _iterate_muon( const LHCb::Detector::DeMuon& muon ) { // test listOfPhysChannels and related code const std::array<LHCb::Detector::DeMuonGasGap, 4> test_all_gaps = det_ch.gaps(); unsigned int i = 0; - double zin, zout; + double zin = 0., zout = 0.; for ( const auto& igap : test_all_gaps ) { i++; @@ -369,7 +369,7 @@ static long test_load_demuon( dd4hep::Detector& description, int argc, char** ar // test listOfPhysChannels and related code const std::array<LHCb::Detector::DeMuonGasGap, 4> test_all_gaps = det_ch.gaps(); unsigned int i = 0; - double zin, zout; + double zin = 0., zout = 0.; for ( const auto& igap : test_all_gaps ) { i++; -- GitLab From a1da50fcb2b0ae6f9eb688f79fc831314d70abbd Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Mon, 31 Jul 2023 14:33:00 +0200 Subject: [PATCH 03/10] [ConditionsGitReader] Limit scope to try block to avoid catching the wrong exceptions --- Core/src/ConditionsGitReader.cpp | 51 +++++++++++++++++--------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/Core/src/ConditionsGitReader.cpp b/Core/src/ConditionsGitReader.cpp index d077f99ce..996a1241a 100644 --- a/Core/src/ConditionsGitReader.cpp +++ b/Core/src/ConditionsGitReader.cpp @@ -95,33 +95,36 @@ int LHCb::Detector::ConditionsGitReader::getObject( const std::string& system_id // Now loading the condition // m_condDB->logger()->level = GitCondDB::Logger::Level::Debug; + std::string data{}; + GitCondDB::CondDB::IOV iov{}; + try { - auto [data, iov] = m_condDB->get( {m_dbtag, path, time_point} ); - - if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) { - if ( iov.since != time_point ) { - throw std::runtime_error( - fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) ); - } - iov.until = iov.since + 1; - } + std::tie( data, iov ) = m_condDB->get( {m_dbtag, path, time_point} ); + } catch ( std::runtime_error const& e ) { return 0; } - buffer = std::move( data ); - // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other, - // and make sure we do not use the part of uint64_t that long does not cover - ct->valid_since = iov.since; // the IOV start is OK by construction - if ( iov.until == GitCondDB::CondDB::IOV::max() ) { - ct->valid_until = dd4hep::IOV::MAX_KEY; - } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) { - throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until, - dd4hep::IOV::MAX_KEY, m_dbtag, path ) ); - } else { - // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV, - // while DD4hep includes it so we have to map [100,200) -> [100,199] - ct->valid_until = iov.until - 1; + if ( !m_limitedIovPaths.empty() && m_limitedIovPaths.find( path ) != m_limitedIovPaths.end() ) { + if ( iov.since != time_point ) { + throw std::runtime_error( + fmt::format( "invalid IOV for {}:{}, cannot find data for point {}", m_dbtag, path, time_point ) ); } - return 1; - } catch ( std::runtime_error const& e ) { return 0; } + iov.until = iov.since + 1; + } + + buffer = std::move( data ); + // GitCondDB uses uint64_t while DD4hep uses long, so we have to map one "max" to the other, + // and make sure we do not use the part of uint64_t that long does not cover + ct->valid_since = iov.since; // the IOV start is OK by construction + if ( iov.until == GitCondDB::CondDB::IOV::max() ) { + ct->valid_until = dd4hep::IOV::MAX_KEY; + } else if ( iov.until >= static_cast<GitCondDB::CondDB::time_point_t>( dd4hep::IOV::MAX_KEY ) ) { + throw std::runtime_error( fmt::format( "invalid IOV boundary ({} >= {}) accessing {}:{}", iov.until, + dd4hep::IOV::MAX_KEY, m_dbtag, path ) ); + } else { + // The -1 is needed because GitCondDB treats upper bounds as not included in the IOV, + // while DD4hep includes it so we have to map [100,200) -> [100,199] + ct->valid_until = iov.until - 1; + } + return 1; } /// Resolve a given URI to a string containing the data -- GitLab From 91d6438da29ff4b2b7cd25ea0dfd0caa647c4b8f Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Mon, 31 Jul 2023 15:36:34 +0200 Subject: [PATCH 04/10] [ConditionsLoader] Print a warning in case a default alignment condition is created --- Core/src/ConditionsLoader.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp index a9dd636a4..2014ef685 100644 --- a/Core/src/ConditionsLoader.cpp +++ b/Core/src/ConditionsLoader.cpp @@ -266,9 +266,9 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, if ( loaded_it == loaded.end() ) { auto [is_alignment, cond] = empty_condition( cond_ident ); register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id ); - dd4hep::printout( dd4hep::DEBUG, "ConditionsLoader", "++ Created empty%s condition %s in %s", - is_alignment ? " alignment" : "", cond_ident->object.c_str(), - cond_ident->sys_id.c_str() ); + dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::DEBUG, "ConditionsLoader", + "++ Created empty%s condition %s in %s", is_alignment ? " alignment" : "", + cond_ident->object.c_str(), cond_ident->sys_id.c_str() ); } } -- GitLab From 43413a41d8655b173d6575bf467ee0cdc1cd77b9 Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Tue, 1 Aug 2023 13:43:26 +0200 Subject: [PATCH 05/10] [DeLHCb] Default conditions allow removal special constructor arguments --- Core/src/DetectorDataService.cpp | 1 - Core/tests/src/test_scope.cpp | 2 +- Detector/LHCb/include/Detector/LHCb/DeLHCb.h | 2 +- Detector/LHCb/src/DeLHCb.cpp | 22 ++++++++------------ 4 files changed, 11 insertions(+), 16 deletions(-) diff --git a/Core/src/DetectorDataService.cpp b/Core/src/DetectorDataService.cpp index e03881d0b..b2f39ffd2 100644 --- a/Core/src/DetectorDataService.cpp +++ b/Core/src/DetectorDataService.cpp @@ -133,7 +133,6 @@ void LHCb::Detector::DetectorDataService::initialize( const nlohmann::json& conf // make sure the detector_names option is in sync auto& opts = *m_description.extension<Options>(); opts["detector_names"] = m_detectorNames; - if ( config.contains( "without_tell40links" ) ) { opts["without_tell40links"] = config.at( "without_tell40links" ); } // Now setup DeLHCb // BEWARE: HAS TO BE DONE BEFORE THE NEXT STEP THAT GATHERS ALL THE CONDITIONS diff --git a/Core/tests/src/test_scope.cpp b/Core/tests/src/test_scope.cpp index 2bcfd626f..d576b59d6 100644 --- a/Core/tests/src/test_scope.cpp +++ b/Core/tests/src/test_scope.cpp @@ -32,7 +32,7 @@ TEST_CASE( "scope_test" ) { description.fromXML( compact_path ); std::vector<std::string> detector_list{"/world", "scope"}; LHCb::Detector::DetectorDataService dds( description, detector_list ); - dds.initialize( nlohmann::json{{"repository", conditions_path}, {"without_tell40links", true}} ); + dds.initialize( nlohmann::json{{"repository", conditions_path}} ); auto slice = dds.get_slice( 100 ); dd4hep::DetElement det = description.detector( "/world/scope/side1/sensor0" ); diff --git a/Detector/LHCb/include/Detector/LHCb/DeLHCb.h b/Detector/LHCb/include/Detector/LHCb/DeLHCb.h index 25a9f0756..e5d9c1b7e 100644 --- a/Detector/LHCb/include/Detector/LHCb/DeLHCb.h +++ b/Detector/LHCb/include/Detector/LHCb/DeLHCb.h @@ -24,7 +24,7 @@ namespace LHCb::Detector { namespace detail { struct DeLHCbObject : DeIOVObject { - DeLHCbObject( const dd4hep::DetElement& de, dd4hep::cond::ConditionUpdateContext& ctxt, bool withTell40Links ); + DeLHCbObject( const dd4hep::DetElement& de, dd4hep::cond::ConditionUpdateContext& ctxt ); void applyToAllChildren( const std::function<void( DeIOVElement<DeIOVObject> )>& ) const override; std::vector<DeIOV> children; Tell40Links m_tell40links; diff --git a/Detector/LHCb/src/DeLHCb.cpp b/Detector/LHCb/src/DeLHCb.cpp index 83700a2c6..8fdd9390c 100644 --- a/Detector/LHCb/src/DeLHCb.cpp +++ b/Detector/LHCb/src/DeLHCb.cpp @@ -18,9 +18,11 @@ #include <stdexcept> LHCb::Detector::detail::DeLHCbObject::DeLHCbObject( const dd4hep::DetElement& de, - dd4hep::cond::ConditionUpdateContext& ctxt, bool withTell40Links ) + dd4hep::cond::ConditionUpdateContext& ctxt ) : DeIOVObject( de, ctxt, 9000000, false ) { - if ( withTell40Links ) { m_tell40links = ctxt.condition( hash_key( de, "Tell40Links" ) ).get<nlohmann::json>(); } + + auto links = ctxt.condition( hash_key( de, "Tell40Links" ) ).get<nlohmann::json>(); + if ( !links.is_null() ) { m_tell40links = links; } } void LHCb::Detector::detail::DeLHCbObject::applyToAllChildren( @@ -45,15 +47,13 @@ namespace { std::vector<std::string> children_names; // Contains the DeIOVs for all the children - bool withTell40Links{true}; - virtual dd4hep::Condition operator()( const dd4hep::ConditionKey& key, dd4hep::cond::ConditionUpdateContext& context ) override final { if ( key.item_key() == LHCb::Detector::Keys::deKey ) { // This is the pointer we need to return at the end of the callback auto lhcbdet = dd4hep::Detector::getInstance().world(); // Creating the Detector element with the desired field - auto delhcbobj = new LHCb::Detector::detail::DeLHCbObject( lhcbdet, context, withTell40Links ); + auto delhcbobj = new LHCb::Detector::detail::DeLHCbObject( lhcbdet, context ); // Now looking up the children DeIOVs for ( const auto& name : children_names ) { LHCb::Detector::DeIOV deiov = LHCb::Detector::detail::get_DeIOV_from_context( context, name ); @@ -96,14 +96,10 @@ void LHCb::Detector::setup_DeLHCb_callback( dd4hep::Detector& description ) { depbuilder.add( hash_key( description.detector( name ), LHCb::Detector::Keys::deKey ) ); } - iovUpdate->withTell40Links = - opts && !( opts->contains( "without_tell40links" ) && opts->at( "without_tell40links" ).get<bool>() ); - if ( iovUpdate->withTell40Links ) { - ( *requests ) - ->addLocation( de, LHCb::Detector::item_key( "Tell40Links" ), "Conditions/LHCb/Online/Tell40Links.yml", - "Tell40Links" ); - depbuilder.add( hash_key( de, "Tell40Links" ) ); - } + ( *requests ) + ->addLocation( de, LHCb::Detector::item_key( "Tell40Links" ), "Conditions/LHCb/Online/Tell40Links.yml", + "Tell40Links" ); + depbuilder.add( hash_key( de, "Tell40Links" ) ); ( *requests )->addDependency( depbuilder.release() ); } -- GitLab From 081ac15bcd92a6045d939a4b94381924fc1df569 Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Tue, 1 Aug 2023 13:45:37 +0200 Subject: [PATCH 06/10] Test default alignment conditions by removing some from the test conditions. Capture and analyse DD4hep printout in a test that uses these conditions. --- CMakeLists.txt | 2 +- Core/tests/src/test_DetectorDataService.cpp | 33 ++++++++++++ TestUtils/CMakeLists.txt | 12 +++-- TestUtils/include/Detector/Test/Fixture.h | 16 +++++- TestUtils/src/Fixture.cpp | 50 +++++++++++++++++++ .../Conditions/VP/Alignment/Global.yml/v1 | 4 -- .../Conditions/VP/Alignment/Global.yml/v2 | 2 - .../ConditionsIOV/Conditions/VP/Motion.yml/0 | 4 -- .../Conditions/VP/Motion.yml/400 | 4 -- 9 files changed, 105 insertions(+), 22 deletions(-) create mode 100644 TestUtils/src/Fixture.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index cd21b78eb..a3f05fb15 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -243,7 +243,7 @@ add_detector_plugin(DetectorTestPlugins Detector/TestScope/scope_cond.cpp Core/tests/src/test_load_scope.cpp ) -target_link_libraries(DetectorTestPlugins GitCondDB::GitCondDB ${FS_LIB} DetectorLib) +target_link_libraries(DetectorTestPlugins GitCondDB::GitCondDB TestUtils ${FS_LIB} DetectorLib) #-------------------------------------------------------------------------- diff --git a/Core/tests/src/test_DetectorDataService.cpp b/Core/tests/src/test_DetectorDataService.cpp index 088614b69..c24df16b1 100644 --- a/Core/tests/src/test_DetectorDataService.cpp +++ b/Core/tests/src/test_DetectorDataService.cpp @@ -19,13 +19,26 @@ #include "Detector/Magnet/DeMagnet.h" #include "Detector/VP/DeVP.h" +#include <Detector/Test/Fixture.h> #include <cstring> #include <exception> #include <nlohmann/json.hpp> +#include <regex> #include <string> #include <vector> +#include <range/v3/action/split.hpp> +#include <range/v3/range/primitives.hpp> +#include <range/v3/view/all.hpp> +#include <range/v3/view/c_str.hpp> +#include <range/v3/view/filter.hpp> + +namespace { + namespace ra = ranges::actions; + namespace rv = ranges::views; +} // namespace + static long test_DetectorDataService( dd4hep::Detector& description, int argc, char** argv ) { bool help = false; @@ -442,11 +455,31 @@ static long test_VP_motion( dd4hep::Detector& description, int argc, char** argv return position; }; + // Use a fixture to capture DD4hep output so we can check if + // warnings about missing alignment conditions were printed + Detector::Test::Fixture f{true}; + // Getting the first position where motion should be 0 auto position100 = get_vpleft_pos( 100 ); + std::cout << "run 100 - VP Left position: " << position100 << '\n'; + auto dd4hep_output = f.printout.str(); + + // Check if warnings were printed about missing alignment conditions + auto lines = ra::split( dd4hep_output, ranges::views::c_str( "\n" ) ); + std::regex missing_align{R"del(WARN\s+.+empty alignment condition)del"}; + std::smatch result; + auto warnings = lines | rv::filter( [&missing_align, &result]( auto line ) { + return std::regex_search( line, result, missing_align ); + } ); + if ( ranges::empty( warnings ) ) { + std::cout << "No warnings printed about missing alignment conditions; expected 2" << std::endl; + ::exit( EINVAL ); + } + auto position1000 = get_vpleft_pos( 1000 ); + std::cout << "run 1000 - VP Left position: " << position1000 << '\n'; auto diff = position1000 - position100; diff --git a/TestUtils/CMakeLists.txt b/TestUtils/CMakeLists.txt index 55d358442..9ba5119de 100644 --- a/TestUtils/CMakeLists.txt +++ b/TestUtils/CMakeLists.txt @@ -10,10 +10,12 @@ ############################################################################### # Library with helper classes to simplify writing tests -add_library(TestUtils INTERFACE) -target_include_directories(TestUtils INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/include) +add_library(TestUtils SHARED src/Fixture.cpp) +target_include_directories(TestUtils PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include) target_link_libraries(TestUtils - INTERFACE - ROOT::Core - DD4hep::DDCore + PRIVATE + fmt::fmt + PUBLIC + ROOT::Core + DD4hep::DDCore ) diff --git a/TestUtils/include/Detector/Test/Fixture.h b/TestUtils/include/Detector/Test/Fixture.h index ba629a7f0..f2a7ab498 100644 --- a/TestUtils/include/Detector/Test/Fixture.h +++ b/TestUtils/include/Detector/Test/Fixture.h @@ -17,11 +17,18 @@ #include <string_view> namespace Detector::Test { + + size_t test_printer( void* fixture, dd4hep::PrintLevel, const char* src, const char* text ); + struct Fixture { // Instantiate the global dd4hep instance and silence printouts - Fixture() : originalLevels{gErrorIgnoreLevel, dd4hep::printLevel()} { + Fixture( bool capture_printout = false ) : originalLevels{gErrorIgnoreLevel, dd4hep::printLevel()} { + auto keep_output = std::getenv( "KEEP_OUTPUT" ); - if ( !keep_output || std::string_view( keep_output ) == "" ) { + + if ( capture_printout ) { + dd4hep::setPrinter( this, &test_printer ); + } else if ( !keep_output || std::string_view( keep_output ) == "" ) { // Set the error level to ignore all messages // - from DD4hep dd4hep::setPrintLevel( static_cast<dd4hep::PrintLevel>( dd4hep::ALWAYS + 1 ) ); @@ -35,6 +42,8 @@ namespace Detector::Test { // Restore the error level gErrorIgnoreLevel = originalLevels.gErrorIgnoreLevel; dd4hep::setPrintLevel( originalLevels.printLevel ); + // Restore the default printer + dd4hep::setPrinter( nullptr, nullptr ); // destroy the global dd4hep instance dd4hep::Detector::destroyInstance(); } @@ -44,6 +53,9 @@ namespace Detector::Test { dd4hep::PrintLevel printLevel; } originalLevels; + std::mutex output_mutex; + std::stringstream printout; + dd4hep::Detector* descriptionPtr{nullptr}; // helpers to access the detector description diff --git a/TestUtils/src/Fixture.cpp b/TestUtils/src/Fixture.cpp new file mode 100644 index 000000000..f2b84d2c9 --- /dev/null +++ b/TestUtils/src/Fixture.cpp @@ -0,0 +1,50 @@ +/*****************************************************************************\ +* (c) Copyright 2022 CERN for the benefit of the LHCb Collaboration * +* * +* This software is distributed under the terms of the GNU General Public * +* Licence version 3 (GPL Version 3), copied verbatim in the file "COPYING". * +* * +* In applying this licence, CERN does not waive the privileges and immunities * +* granted to it by virtue of its status as an Intergovernmental Organization * +* or submit itself to any jurisdiction. * +\*****************************************************************************/ +#include <Detector/Test/Fixture.h> +#include <fmt/format.h> + +namespace { + + // Copied from DD4hep, because it's not available outside it... + const char* print_level( dd4hep::PrintLevel lvl ) { + switch ( lvl ) { + case dd4hep::NOLOG: + return "NOLOG"; + case dd4hep::VERBOSE: + return "VERB "; + case dd4hep::DEBUG: + return "DEBUG"; + case dd4hep::INFO: + return "INFO "; + case dd4hep::WARNING: + return "WARN "; + case dd4hep::ERROR: + return "ERROR"; + case dd4hep::FATAL: + return "FATAL"; + case dd4hep::ALWAYS: + return " "; + default: + if ( lvl > dd4hep::ALWAYS ) return print_level( dd4hep::ALWAYS ); + return print_level( dd4hep::NOLOG ); + } + } +} // namespace + +size_t Detector::Test::test_printer( void* fixture, dd4hep::PrintLevel lvl, const char* src, const char* text ) { + Fixture* f = reinterpret_cast<Fixture*>( fixture ); + std::lock_guard<std::mutex> lock( f->output_mutex ); + // std::string s = std::string{src} + " " + text; + std::string s = fmt::format( "{:-<16} {} {}", src, print_level( lvl ), text ); + f->printout << s << "\n"; + std::cout << s << "\n"; + return s.size() + 1; +} diff --git a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1 b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1 index 32c5f2898..515d5dbe9 100644 --- a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1 +++ b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v1 @@ -1,6 +1,2 @@ VPSystem: !alignment position: [1.0 * mm, 0.1 * cm, 1000 * um] -VPLeft: !alignment - position: [0.0 * mm, 0.0 * mm, 0.0 * mm] -VPRight: !alignment - position: [0.0 * mm, 0.0 * mm, 0.0 * mm] diff --git a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2 b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2 index fcbf9690f..287dec0b3 100644 --- a/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2 +++ b/tests/ConditionsIOV/Conditions/VP/Alignment/Global.yml/v2 @@ -1,6 +1,4 @@ VPSystem: !alignment position: [1.0 * mm, 1.0 * mm, 1.0 * mm] -VPLeft: !alignment - position: [0.0 * mm, 0.0 * mm, 0.0 * mm] VPRight: !alignment position: [5.0 * mm, 5.0 * mm, 5.0 * mm] diff --git a/tests/ConditionsIOV/Conditions/VP/Motion.yml/0 b/tests/ConditionsIOV/Conditions/VP/Motion.yml/0 index 8d48475d2..e69de29bb 100644 --- a/tests/ConditionsIOV/Conditions/VP/Motion.yml/0 +++ b/tests/ConditionsIOV/Conditions/VP/Motion.yml/0 @@ -1,4 +0,0 @@ -MotionVPLeft: !alignment - position: [0.0 * mm, 0.0 * mm, 0.0 * mm] -MotionVPRight: !alignment - position: [0.0 * mm, 0.0 * mm, 0.0 * mm] diff --git a/tests/ConditionsIOV/Conditions/VP/Motion.yml/400 b/tests/ConditionsIOV/Conditions/VP/Motion.yml/400 index 8d48475d2..e69de29bb 100644 --- a/tests/ConditionsIOV/Conditions/VP/Motion.yml/400 +++ b/tests/ConditionsIOV/Conditions/VP/Motion.yml/400 @@ -1,4 +0,0 @@ -MotionVPLeft: !alignment - position: [0.0 * mm, 0.0 * mm, 0.0 * mm] -MotionVPRight: !alignment - position: [0.0 * mm, 0.0 * mm, 0.0 * mm] -- GitLab From 3c003d1e6a4b0e67213725223ee81fa2c6e53fe9 Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Wed, 2 Aug 2023 14:30:41 +0200 Subject: [PATCH 07/10] Use tags to identify indices of locations stored in the conditions repository. --- Core/include/Core/ConditionsRepository.h | 21 ++++++++++++- Core/src/ConditionsLoader.cpp | 13 ++++---- Core/src/ConditionsRepository.cpp | 38 +++++++++++------------- Core/src/DetectorDataService.cpp | 9 +++--- 4 files changed, 50 insertions(+), 31 deletions(-) diff --git a/Core/include/Core/ConditionsRepository.h b/Core/include/Core/ConditionsRepository.h index 97814a43e..2f5c36071 100644 --- a/Core/include/Core/ConditionsRepository.h +++ b/Core/include/Core/ConditionsRepository.h @@ -29,6 +29,14 @@ namespace { namespace LHCb::Detector { + // Tag structs to refer to the three indices of the + // ConditionsRepository::Locations multi-index container + struct by_location_hash {}; + struct by_filename_hash {}; + struct by_condition_key {}; + + /// Utility function to extract the combined location hash (filename + /// and condition name) from a ConditionIdentifier dd4hep::Condition::key_type location_key( const ConditionIdentifier* entry ); struct RequestSelector; @@ -42,13 +50,24 @@ namespace LHCb::Detector { friend struct RequestSelector; public: + // A multi-index container that allows (fast) lookup in three + // ways. the value is always the ConditionIdentifier. using Locations = boost::multi_index_container< ConditionIdentifier*, bmi::indexed_by< + // Analogously to an `std::map` with key the combined hash + // of the filename and the condition name bmi::ordered_unique< + bmi::tag<LHCb::Detector::by_location_hash>, bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &location_key>>, - bmi::hashed_non_unique<bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>, + // Analogously to an `std::unordered_multimap` with key the hash of the filename + bmi::hashed_non_unique<bmi::tag<LHCb::Detector::by_filename_hash>, + bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>, + // Analogously to an `std::unordered_map` with key the + // combined hash of the detector element and the condition + // name bmi::hashed_unique< + bmi::tag<LHCb::Detector::by_condition_key>, bmi::member<ConditionIdentifier, dd4hep::Condition::key_type, &ConditionIdentifier::hash>>>>; Locations locations; diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp index 2014ef685..c651b522c 100644 --- a/Core/src/ConditionsLoader.cpp +++ b/Core/src/ConditionsLoader.cpp @@ -236,8 +236,8 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, const std::string cond_name = cond_data.first.as<std::string>(); // see if the condition is in the list of locations(?) - // Only the lower 32 bits of kmaker.hash have been set - // above, set the upper bits here + // Only the upper 32 bits of kmaker.hash have been set + // above, set the lower bits here kmaker.values.item_key = dd4hep::detail::hash32( cond_name ); auto cond_ident = repo->locations.find( kmaker.hash ); // O[log(N)] @@ -260,8 +260,8 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // Check if there are conditions in this file that have been // requested, but have not yet been loaded. If so, create // empty conditions for those too. - for ( const auto* cond_ident : - boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) { + for ( const auto* cond_ident : boost::make_iterator_range( + repo->locations.get<by_filename_hash>().equal_range( cond_id->sys_id_hash ) ) ) { auto loaded_it = loaded.find( cond_ident->hash ); if ( loaded_it == loaded.end() ) { auto [is_alignment, cond] = empty_condition( cond_ident ); @@ -302,10 +302,11 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // conditions for all requested conditions that are in this // file. std::vector<dd4hep::Condition> entity_conditions; - entity_conditions.reserve( repo->locations.get<1>().count( cond_id->sys_id_hash ) ); + auto const& conditions_by_filename_hash = repo->locations.get<by_filename_hash>(); + entity_conditions.reserve( conditions_by_filename_hash.count( cond_id->sys_id_hash ) ); for ( const auto* cond_ident : - boost::make_iterator_range( repo->locations.get<1>().equal_range( cond_id->sys_id_hash ) ) ) { + boost::make_iterator_range( conditions_by_filename_hash.equal_range( cond_id->sys_id_hash ) ) ) { auto [is_alignment, cond] = empty_condition( cond_ident ); register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id ); } diff --git a/Core/src/ConditionsRepository.cpp b/Core/src/ConditionsRepository.cpp index 8a61ac149..6e06712d3 100644 --- a/Core/src/ConditionsRepository.cpp +++ b/Core/src/ConditionsRepository.cpp @@ -43,16 +43,15 @@ LHCb::Detector::ConditionsRepository::addLocation( dd4hep::DetElement de, dd4hep const std::string& sys_id, const std::string& object ) { ConditionIdentifier ident; dd4hep::ConditionKey::KeyMaker km( de.key(), item ); - ident.hash = km.hash; - ident.detector = de; - ident.object = object; - ident.sys_id = sys_id; - ident.sys_id_hash = dd4hep::detail::hash32( sys_id ); - ident.object_hash = dd4hep::detail::hash32( object ); - ident.repository = this; - auto* info = new LoadInfo<ConditionIdentifier>( ident ); - auto ret = addLocationInfo( km.hash, info ); - dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash ); + ident.hash = km.hash; + ident.detector = de; + ident.object = object; + ident.sys_id = sys_id; + ident.sys_id_hash = dd4hep::detail::hash32( sys_id ); + ident.object_hash = dd4hep::detail::hash32( object ); + ident.repository = this; + auto* info = new LoadInfo<ConditionIdentifier>( ident ); + auto ret = addLocationInfo( km.hash, info ); locations.insert( &info->info ); return ret; } @@ -62,16 +61,15 @@ LHCb::Detector::ConditionsRepository::addGlobal( dd4hep::DetElement de, dd4hep:: const std::string& sys_id, const std::string& object ) { ConditionIdentifier ident; dd4hep::ConditionKey::KeyMaker km( 0, item ); - ident.hash = km.hash; - ident.detector = de; - ident.object = object; - ident.sys_id = sys_id; - ident.sys_id_hash = dd4hep::detail::hash32( sys_id ); - ident.object_hash = dd4hep::detail::hash32( object ); - ident.repository = this; - auto* info = new LoadInfo<ConditionIdentifier>( ident ); - auto ret = addLocationInfo( km.hash, info ); - dd4hep::ConditionKey::KeyMaker loc( ident.sys_id_hash, ident.object_hash ); + ident.hash = km.hash; + ident.detector = de; + ident.object = object; + ident.sys_id = sys_id; + ident.sys_id_hash = dd4hep::detail::hash32( sys_id ); + ident.object_hash = dd4hep::detail::hash32( object ); + ident.repository = this; + auto* info = new LoadInfo<ConditionIdentifier>( ident ); + auto ret = addLocationInfo( km.hash, info ); locations.insert( &info->info ); return ret; } diff --git a/Core/src/DetectorDataService.cpp b/Core/src/DetectorDataService.cpp index b2f39ffd2..3b308f648 100644 --- a/Core/src/DetectorDataService.cpp +++ b/Core/src/DetectorDataService.cpp @@ -181,8 +181,8 @@ void LHCb::Detector::DetectorDataService::initialize( const nlohmann::json& conf auto cond_key = LHCb::Detector::ConditionKey( m_description.detector( std::string{deName} ), std::string{condKey} ); // Check in the conditions mappings which file this corresponds to - if ( auto el = m_all_conditions->locations.get<2>().find( cond_key ); - el != end( m_all_conditions->locations.get<2>() ) ) { + const auto& conditions_by_key = m_all_conditions->locations.get<by_condition_key>(); + if ( auto el = conditions_by_key.find( cond_key ); el != end( conditions_by_key ) ) { overrides.emplace_back( LHCb::Detector::ConditionOverrideEntry{( *el )->sys_id, ( *el )->object, YAML::Load( o.second )} ); } else { @@ -322,8 +322,9 @@ void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetEle void LHCb::Detector::DetectorDataService::update_condition( const dd4hep::DetElement& de, const std::string& cond, const YAML::Node& data ) { - auto key = LHCb::Detector::ConditionKey( de, cond ); - if ( auto el = m_all_conditions->locations.get<2>().find( key ); el != end( m_all_conditions->locations.get<2>() ) ) { + auto key = LHCb::Detector::ConditionKey( de, cond ); + const auto& conditions_by_key = m_all_conditions->locations.get<by_condition_key>(); + if ( auto el = conditions_by_key.find( key ); el != end( conditions_by_key ) ) { update_condition( ( *el )->sys_id, ( *el )->object, data ); } else { throw std::invalid_argument{"cannot find " + cond + " in " + de.name() + "'s ConditionsRepository"}; -- GitLab From b444c12a886b8f5aaa63e670b257b7863230b8b5 Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Wed, 2 Aug 2023 14:31:32 +0200 Subject: [PATCH 08/10] Print info message instead of debug when creating a missing classical condition. --- Core/src/ConditionsLoader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp index c651b522c..c46a13814 100644 --- a/Core/src/ConditionsLoader.cpp +++ b/Core/src/ConditionsLoader.cpp @@ -266,7 +266,7 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, if ( loaded_it == loaded.end() ) { auto [is_alignment, cond] = empty_condition( cond_ident ); register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id ); - dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::DEBUG, "ConditionsLoader", + dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::INFO, "ConditionsLoader", "++ Created empty%s condition %s in %s", is_alignment ? " alignment" : "", cond_ident->object.c_str(), cond_ident->sys_id.c_str() ); } -- GitLab From 3b0423e60793da5ffccab3db80078a3cc16b9f9c Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Wed, 2 Aug 2023 15:06:43 +0200 Subject: [PATCH 09/10] Use rad instead of mm for rotations --- Core/src/ConditionsLoader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp index c46a13814..93dcb85ff 100644 --- a/Core/src/ConditionsLoader.cpp +++ b/Core/src/ConditionsLoader.cpp @@ -51,7 +51,7 @@ namespace { [&key]( auto hash ) { return key.values.item_key == hash; } ); auto const* payload = is_alignment ? "NotFound: !alignment\n position: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n " - "rotation: [0.0 * mm, 0.0 * mm, 0.0 * mm]\n" + "rotation: [0.0 * rad, 0.0 * rad, 0.0 * rad]\n" : ""; auto node = YAML::Load( payload ); return {is_alignment, is_alignment ? node["NotFound"] : node}; -- GitLab From 2f9c575412398aaecd8d2b1358e9b871815e6ec9 Mon Sep 17 00:00:00 2001 From: Roel Aaij <roel.aaij@cern.ch> Date: Thu, 3 Aug 2023 12:51:39 +0200 Subject: [PATCH 10/10] [ConditionsLoader] loop over filenames instead of conditions --- Core/include/Core/ConditionsRepository.h | 25 ++-- Core/src/ConditionsLoader.cpp | 140 ++++++++--------------- 2 files changed, 67 insertions(+), 98 deletions(-) diff --git a/Core/include/Core/ConditionsRepository.h b/Core/include/Core/ConditionsRepository.h index 2f5c36071..87c6e28b2 100644 --- a/Core/include/Core/ConditionsRepository.h +++ b/Core/include/Core/ConditionsRepository.h @@ -20,6 +20,7 @@ #include <boost/multi_index/hashed_index.hpp> #include <boost/multi_index/key_extractors.hpp> +#include <boost/multi_index/member.hpp> #include <boost/multi_index/ordered_index.hpp> #include <boost/multi_index_container.hpp> @@ -32,13 +33,19 @@ namespace LHCb::Detector { // Tag structs to refer to the three indices of the // ConditionsRepository::Locations multi-index container struct by_location_hash {}; - struct by_filename_hash {}; + struct by_filename {}; struct by_condition_key {}; /// Utility function to extract the combined location hash (filename /// and condition name) from a ConditionIdentifier dd4hep::Condition::key_type location_key( const ConditionIdentifier* entry ); + /// Utility function to extract the filename hash from a const ConditionIdentifier + inline std::string condition_filename( const ConditionIdentifier* entry ) { return entry->sys_id; }; + + /// Utility function to extract the condition key from a const ConditionIdentifier + inline dd4hep::Condition::key_type condition_key( const ConditionIdentifier* entry ) { return entry->hash; }; + struct RequestSelector; /** @@ -51,25 +58,27 @@ namespace LHCb::Detector { public: // A multi-index container that allows (fast) lookup in three - // ways. the value is always the ConditionIdentifier. + // ways. the value is either ConditionIdentifier* or + // const ConditionIdentifier*. + template <bool const_pointer = false> using Locations = boost::multi_index_container< - ConditionIdentifier*, + std::conditional_t<const_pointer, const ConditionIdentifier*, ConditionIdentifier*>, bmi::indexed_by< // Analogously to an `std::map` with key the combined hash // of the filename and the condition name bmi::ordered_unique< bmi::tag<LHCb::Detector::by_location_hash>, bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &location_key>>, - // Analogously to an `std::unordered_multimap` with key the hash of the filename - bmi::hashed_non_unique<bmi::tag<LHCb::Detector::by_filename_hash>, - bmi::member<ConditionIdentifier, int, &ConditionIdentifier::sys_id_hash>>, + // Analogously to an `std::multimap` with key the hash of the filename + bmi::ordered_non_unique<bmi::tag<LHCb::Detector::by_filename>, + bmi::global_fun<const ConditionIdentifier*, std::string, &condition_filename>>, // Analogously to an `std::unordered_map` with key the // combined hash of the detector element and the condition // name bmi::hashed_unique< bmi::tag<LHCb::Detector::by_condition_key>, - bmi::member<ConditionIdentifier, dd4hep::Condition::key_type, &ConditionIdentifier::hash>>>>; - Locations locations; + bmi::global_fun<const ConditionIdentifier*, dd4hep::Condition::key_type, &condition_key>>>>; + Locations<> locations; private: /// Default assignment operator diff --git a/Core/src/ConditionsLoader.cpp b/Core/src/ConditionsLoader.cpp index 93dcb85ff..de17b0d59 100644 --- a/Core/src/ConditionsLoader.cpp +++ b/Core/src/ConditionsLoader.cpp @@ -40,6 +40,10 @@ #include <utility> #include <vector> +#include <range/v3/view/all.hpp> +#include <range/v3/view/transform.hpp> +#include <range/v3/view/unique.hpp> + namespace { std::tuple<bool, YAML::Node> empty_condition( const LHCb::Detector::ConditionIdentifier* cond ) { const std::array<dd4hep::Condition::key_type, 2> alignment_keys = {LHCb::Detector::Keys::deltaKey, @@ -56,6 +60,9 @@ namespace { auto node = YAML::Load( payload ); return {is_alignment, is_alignment ? node["NotFound"] : node}; } + + namespace rv = ranges::views; + namespace ra = ranges::actions; } // namespace /// Standard constructor, initializes variables @@ -110,13 +117,6 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // via "SYSTEM" keyword (needed only for XML parsing but done here instead of inside the loop) dd4hep::xml::UriContextReader xml_entity_resolver( m_reader.get(), &ctxt ); - // First collect all required URIs which need loading. - // Since one file contains many conditions, we have - // to create a unique set - - // container to keep track of the URLs already visited - std::set<std::string_view> processed_urls; - // FIXME using a stable buffer for storing the condition docs to be parsed might look like an optimization, // but in the case of GitCondDB (or the ConditionsFileReader) it does not really help as each request // creates a new string that then has to be moved into this buffer @@ -126,19 +126,22 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // dd4hep::setPrintLevel( dd4hep::DEBUG ); // I have the impression ConditionsIdentifier is not a very good name, but I stick by it for the moment - struct ToDoItem { - ToDoItem( const dd4hep::cond::ConditionsLoadInfo* load_info ) - : cond_id{reinterpret_cast<const ConditionIdentifier*>( load_info->ptr() )} {} - bool done = false; - const ConditionIdentifier* cond_id = nullptr; - }; + ConditionsRepository::Locations<true> all_conditions{}; + std::for_each( conditions_to_load.begin(), conditions_to_load.end(), [&all_conditions]( auto entry ) { + auto* load_info = entry.second->ptr(); + if ( !load_info ) { + dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ CANNOT update condition: [%16llX] [No load info]", + entry.first ); + } else { + all_conditions.insert( reinterpret_cast<const ConditionIdentifier*>( load_info ) ); + } + } ); + + const auto& conditions_by_location_hash = all_conditions.get<by_location_hash>(); + const auto& conditions_by_filename = all_conditions.get<by_filename>(); - // we convert the list of conditions to load into a map so that we can easily - // flag as already loaded the conditions we get from each URL that are not - // what we requested initially. So, if we ask for conditions A and B, and - // when processing A we load B as it comes from the same URL, then we flag B - // as done so that we can skip the iteration. - std::map<dd4hep::Condition::key_type, ToDoItem> todo_list{conditions_to_load.begin(), conditions_to_load.end()}; + // Extract the range of unique filenames from the conditions + auto unique_filenames = conditions_by_filename | rv::transform( &ConditionIdentifier::sys_id ) | rv::unique; auto block_register = [this]( dd4hep::IOV::Key iov_key, const std::vector<dd4hep::Condition>& entity_conditions ) { dd4hep::cond::ConditionsPool* pool = manager().registerIOV( *m_iovType, iov_key ); @@ -146,18 +149,16 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, }; try { - for ( auto [current_key, item] : todo_list ) { // O[N] + for ( auto filename : unique_filenames ) { - auto register_condition = [&loaded, &todo_list, ¤t_key = current_key, - &item = item]( std::vector<dd4hep::Condition>& entity_conditions, - const ConditionIdentifier* cond_ident, std::string cond_name, - const YAML::Node& cond_data, std::string doc ) { + auto register_condition = [&loaded]( std::vector<dd4hep::Condition>& entity_conditions, + const ConditionIdentifier* cond_ident, const std::string& cond_name, + const YAML::Node& cond_data, const std::string& doc ) { dd4hep::Condition c = LHCb::YAMLConverters::make_condition( cond_name, cond_data ); if ( !c.isValid() ) { // Will eventually have to raise an exception dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Failed to load condition %s from %s", cond_name.c_str(), doc.c_str() ); - return false; } else { const auto cond_key = cond_ident->hash; @@ -167,59 +168,28 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, // record the condition for bulk registration to the manager entity_conditions.push_back( c ); - - // if this is the condition we were looking for, - // flag the ToDoItem as done and move to the next condition in the YAML object - if ( cond_key == current_key ) { - item.done = true; - return false; - } - // otherwise see if we are supposed to load it, - // in which case the pending work item can be marked as already dine - if ( auto cond_itr = todo_list.find( cond_key ); cond_itr != todo_list.end() ) { - cond_itr->second.done = true; - return false; - } - return true; } }; - const auto cond_id = item.cond_id; - // FIXME this early skip for conditions already loaded from a URL does not convince me - if ( item.done ) { continue; } - if ( !cond_id ) { - dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ CANNOT update condition: [%16llX] [No load info]", - current_key ); - continue; - } - - const auto* repo = cond_id->repository; - - // set::insert returns a pair where the second tells if the insertion - // took place (true) or the entry was already present (false) - if ( !processed_urls.insert( cond_id->sys_id ).second ) { - // no insertion - continue; - } - // ready, set, chrono started! TTimeStamp start; // Load from the git cond db here XXX // If the file can't be loaded, create empty conditions for all // of the conditions expected in this file - if ( m_reader->load( cond_id->sys_id, &ctxt, buffer ) ) { - auto ext = std::string_view{cond_id->sys_id}; + if ( m_reader->load( filename, &ctxt, buffer ) ) { + auto ext = std::string_view{filename}; ext.remove_prefix( ext.find_last_of( '.' ) ); if ( ext == ".yml" || ext == ".yaml" ) { - dd4hep::ConditionKey::KeyMaker kmaker( cond_id->sys_id_hash, 0 ); + // Use the filename hash as the upper bits of the hash + dd4hep::ConditionKey::KeyMaker kmaker( dd4hep::detail::hash32( filename ), 0 ); // condition YAML files are expected to contain a mapping <name> -> <condition data> - auto data = YAML::Load( ( m_useOverlay ) ? m_overlay.get_or_default( cond_id->sys_id, buffer ) : buffer ); + auto data = YAML::Load( ( m_useOverlay ) ? m_overlay.get_or_default( filename, buffer ) : buffer ); // Checking whether we have conditions to override in that same file (i.e. o.path) // in that case we directly update the YAML node for ( auto const& o : m_conditionsOverride ) { - if ( o.path == cond_id->sys_id ) { + if ( o.path == filename ) { dd4hep::printout( dd4hep::DEBUG, "ConditionsLoader", "++ Overriding %s:%s", o.path.c_str(), o.name.c_str() ); data[o.name] = o.value; @@ -235,37 +205,29 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, for ( const auto& cond_data : data ) { const std::string cond_name = cond_data.first.as<std::string>(); - // see if the condition is in the list of locations(?) + // Check if the condition has been requested. // Only the upper 32 bits of kmaker.hash have been set // above, set the lower bits here kmaker.values.item_key = dd4hep::detail::hash32( cond_name ); - auto cond_ident = repo->locations.find( kmaker.hash ); // O[log(N)] - if ( cond_ident == repo->locations.end() ) { - dd4hep::printout( dd4hep::ERROR, "ConditionsLoader", "++ Got stray condition %s from %s", - cond_name.c_str(), cond_id->sys_id.c_str() ); - continue; + auto cond_ident = conditions_by_location_hash.find( kmaker.hash ); // O[log(N)] + if ( cond_ident != conditions_by_location_hash.end() ) { + register_condition( entity_conditions, *cond_ident, cond_name, cond_data.second, filename ); + } else { + dd4hep::printout( dd4hep::WARNING, "ConditionsLoader", "++ Got stray condition %s from %s", + cond_name.c_str(), filename.c_str() ); } - - bool stray = - register_condition( entity_conditions, *cond_ident, cond_name, cond_data.second, cond_id->sys_id ); - if ( !stray ) continue; - - // this warning means that we found in the YAML file a condition we were not requesting - // FIXME I do not see why this is a problem - dd4hep::printout( dd4hep::WARNING, "ConditionsLoader", "++ Got stray but valid condition %s from %s", - cond_name.c_str(), cond_id->sys_id.c_str() ); } // Check if there are conditions in this file that have been // requested, but have not yet been loaded. If so, create // empty conditions for those too. - for ( const auto* cond_ident : boost::make_iterator_range( - repo->locations.get<by_filename_hash>().equal_range( cond_id->sys_id_hash ) ) ) { + for ( const auto* cond_ident : + boost::make_iterator_range( conditions_by_filename.equal_range( filename ) ) ) { auto loaded_it = loaded.find( cond_ident->hash ); if ( loaded_it == loaded.end() ) { auto [is_alignment, cond] = empty_condition( cond_ident ); - register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id ); + register_condition( entity_conditions, cond_ident, "NotFound", cond, filename ); dd4hep::printout( is_alignment ? dd4hep::WARNING : dd4hep::INFO, "ConditionsLoader", "++ Created empty%s condition %s in %s", is_alignment ? " alignment" : "", cond_ident->object.c_str(), cond_ident->sys_id.c_str() ); @@ -292,23 +254,21 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, std::string valid_end = ctxt.valid_until == std::numeric_limits<std::int64_t>::max() ? std::string{"inf "} : std::to_string( ctxt.valid_until ); - dd4hep::printout( - dd4hep::INFO, "ConditionsLoader", "++ Added %4ld conditions from %-48s to pool [%6s-%6s] [%7.3f sec]", - loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( ctxt.valid_since ).c_str(), - valid_end.c_str(), stop.AsDouble() - start.AsDouble() ); + dd4hep::printout( dd4hep::INFO, "ConditionsLoader", + "++ Added %4ld conditions from %-48s to pool [%6s-%6s] [%7.3f sec]", + loaded.size() - loaded_len, filename.c_str(), std::to_string( ctxt.valid_since ).c_str(), + valid_end.c_str(), stop.AsDouble() - start.AsDouble() ); loaded_len = loaded.size(); } else { // File containing Conditions not found, create empty // conditions for all requested conditions that are in this // file. std::vector<dd4hep::Condition> entity_conditions; - auto const& conditions_by_filename_hash = repo->locations.get<by_filename_hash>(); - entity_conditions.reserve( conditions_by_filename_hash.count( cond_id->sys_id_hash ) ); + entity_conditions.reserve( conditions_by_filename.count( filename ) ); - for ( const auto* cond_ident : - boost::make_iterator_range( conditions_by_filename_hash.equal_range( cond_id->sys_id_hash ) ) ) { + for ( const auto* cond_ident : boost::make_iterator_range( conditions_by_filename.equal_range( filename ) ) ) { auto [is_alignment, cond] = empty_condition( cond_ident ); - register_condition( entity_conditions, cond_ident, "NotFound", cond, cond_id->sys_id ); + register_condition( entity_conditions, cond_ident, "NotFound", cond, filename ); } // If the file is not in the DB, it's never going to appear, @@ -317,7 +277,7 @@ size_t LHCb::Detector::ConditionsLoader::load_many( const dd4hep::IOV& req_iov, block_register( iov_key, entity_conditions ); dd4hep::printout( dd4hep::INFO, "ConditionsLoader", "++ Added %4ld null conds for %-48s to pool [%6s-%6s]", - loaded.size() - loaded_len, cond_id->sys_id.c_str(), std::to_string( 0 ).c_str(), "inf " ); + loaded.size() - loaded_len, filename.c_str(), std::to_string( 0 ).c_str(), "inf " ); loaded_len = loaded.size(); } if ( print_results ) { -- GitLab