Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Centralise payload ptr ownership for condition updating workflows #35048

Merged
merged 5 commits into from Aug 31, 2021
Merged

Centralise payload ptr ownership for condition updating workflows #35048

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b86c5be
Implement centralized pointer ownership and reference-based interface
ggovi Aug 27, 2021
24dc549
Implement centralized payload pointer ownership and shared_ptr based …
ggovi Aug 27, 2021
beace2e
Adapted to new PopCon interface
ggovi Aug 27, 2021
40f6c27
Moving clients holding the payload pointer ownership to the new DBOup…
ggovi Aug 27, 2021
0eada5d
Cosmetics on comments...
ggovi Aug 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions CalibCalorimetry/CaloMiscalibTools/plugins/WriteEcalMiscalibConstants.cc
View file
Open in desktop
Expand Up @@ -69,12 +69,12 @@ void WriteEcalMiscalibConstants::analyze(const edm::Event& iEvent, const edm::Ev
if (poolDbService.isAvailable()) {
if (poolDbService->isNewTagRequest(newTagRequest_)) {
std::cout << " Creating a new one " << std::endl;
poolDbService->createNewIOV<const EcalIntercalibConstants>(
Mcal, poolDbService->beginOfTime(), poolDbService->endOfTime(), newTagRequest_);
poolDbService->createNewIOV<const EcalIntercalibConstants>(*Mcal, poolDbService->beginOfTime(), newTagRequest_);
std::cout << "Done" << std::endl;
} else {
std::cout << "Old One " << std::endl;
poolDbService->appendSinceTime<const EcalIntercalibConstants>(Mcal, poolDbService->currentTime(), newTagRequest_);
poolDbService->appendSinceTime<const EcalIntercalibConstants>(
*Mcal, poolDbService->currentTime(), newTagRequest_);
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions CalibCalorimetry/CaloMiscalibTools/plugins/WriteEcalMiscalibConstantsMC.cc
View file
Open in desktop
Expand Up @@ -69,13 +69,12 @@ void WriteEcalMiscalibConstantsMC::analyze(const edm::Event& iEvent, const edm::
if (poolDbService.isAvailable()) {
if (poolDbService->isNewTagRequest(newTagRequest_)) {
std::cout << " Creating a new one " << std::endl;
poolDbService->createNewIOV<const EcalIntercalibConstantsMC>(
Mcal, poolDbService->beginOfTime(), poolDbService->endOfTime(), newTagRequest_);
poolDbService->createNewIOV<const EcalIntercalibConstantsMC>(*Mcal, poolDbService->beginOfTime(), newTagRequest_);
std::cout << "Done" << std::endl;
} else {
std::cout << "Old One " << std::endl;
poolDbService->appendSinceTime<const EcalIntercalibConstantsMC>(
Mcal, poolDbService->currentTime(), newTagRequest_);
*Mcal, poolDbService->currentTime(), newTagRequest_);
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions CalibTracker/SiPixelQuality/plugins/SiPixelStatusHarvester.cc
View file
Open in desktop
Expand Up @@ -194,7 +194,7 @@ void SiPixelStatusHarvester::dqmEndRun(const edm::Run& iRun, const edm::EventSet
}
if (debug_ == true) { // only produced for debugging reason
cond::Time_t thisIOV = (cond::Time_t)iRun.id().run();
poolDbService->writeOne<SiPixelQuality>(siPixelQualityPermBad, thisIOV, recordName_ + "_permanentBad");
poolDbService->writeOne<SiPixelQuality>(*siPixelQualityPermBad, thisIOV, recordName_ + "_permanentBad");
}

// IOV for final payloads. FEDerror25 and pcl
Expand Down Expand Up @@ -298,7 +298,7 @@ void SiPixelStatusHarvester::dqmEndRun(const edm::Run& iRun, const edm::EventSet
fedError25IOV[it->first] = it->first;

if (debug_ == true) // only produced for debugging reason
poolDbService->writeOne<SiPixelQuality>(siPixelQuality_FEDerror25, thisIOV, recordName_ + "_FEDerror25");
poolDbService->writeOne<SiPixelQuality>(*siPixelQuality_FEDerror25, thisIOV, recordName_ + "_FEDerror25");

delete siPixelQuality_FEDerror25;
}
Expand Down Expand Up @@ -556,12 +556,12 @@ void SiPixelStatusHarvester::dqmEndRun(const edm::Run& iRun, const edm::EventSet
edm::LuminosityBlockID lu(iRun.id().run(), endLumiBlock_ + 1);
cond::Time_t thisIOV = (cond::Time_t)(lu.value());
if (!SiPixelStatusHarvester::equal(lastPrompt, siPixelQualityPermBad))
poolDbService->writeOne<SiPixelQuality>(siPixelQualityPermBad, thisIOV, recordName_ + "_prompt");
poolDbService->writeOne<SiPixelQuality>(*siPixelQualityPermBad, thisIOV, recordName_ + "_prompt");

// add empty bad components to last lumi+1 IF AND ONLY IF the last payload of other is not equal to empty
SiPixelQuality* siPixelQualityDummy = new SiPixelQuality();
if (!SiPixelStatusHarvester::equal(lastOther, siPixelQualityDummy))
poolDbService->writeOne<SiPixelQuality>(siPixelQualityDummy, thisIOV, recordName_ + "_other");
poolDbService->writeOne<SiPixelQuality>(*siPixelQualityDummy, thisIOV, recordName_ + "_other");

delete siPixelQualityDummy;
}
Expand Down Expand Up @@ -657,12 +657,12 @@ void SiPixelStatusHarvester::constructTag(std::map<int, SiPixelQuality*> siPixel

SiPixelQuality* thisPayload = qIt->second;
if (qIt == siPixelQualityTag.begin())
poolDbService->writeOne<SiPixelQuality>(thisPayload, thisIOV, recordName_ + "_" + tagName);
poolDbService->writeOne<SiPixelQuality>(*thisPayload, thisIOV, recordName_ + "_" + tagName);
else {
SiPixelQuality* prevPayload = (std::prev(qIt))->second;
if (!SiPixelStatusHarvester::equal(thisPayload,
prevPayload)) // only append newIOV if this payload differs wrt last
poolDbService->writeOne<SiPixelQuality>(thisPayload, thisIOV, recordName_ + "_" + tagName);
poolDbService->writeOne<SiPixelQuality>(*thisPayload, thisIOV, recordName_ + "_" + tagName);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion CalibTracker/SiStripESProducers/plugins/DBWriter/DummyCondDBWriter.h
View file
Open in desktop
Expand Up @@ -68,7 +68,7 @@ void DummyCondDBWriter<TObject, TObjectO, TRecord>::endRun(const edm::Run& run,
else
Time_ = iConfig_.getUntrackedParameter<uint32_t>("OpenIovAtTime", 1);

dbservice->writeOne(obj.release(), Time_, rcdName);
dbservice->writeOne(*obj, Time_, rcdName);
} else {
edm::LogError("SiStripFedCablingBuilder") << "Service is unavailable" << std::endl;
}
Expand Down
4 changes: 2 additions & 2 deletions CalibTracker/SiStripESProducers/plugins/DBWriter/SiStripFedCablingManipulator.cc
View file
Open in desktop
Expand Up @@ -41,12 +41,12 @@ void SiStripFedCablingManipulator::endRun(const edm::Run& run, const edm::EventS
edm::LogInfo("SiStripFedCablingManipulator") << "first request for storing objects with Record "
<< "SiStripFedCablingRcd"
<< " at time " << Time_ << std::endl;
dbservice->createNewIOV<SiStripFedCabling>(obj.release(), Time_, dbservice->endOfTime(), "SiStripFedCablingRcd");
dbservice->createNewIOV<SiStripFedCabling>(*obj, Time_, "SiStripFedCablingRcd");
} else {
edm::LogInfo("SiStripFedCablingManipulator") << "appending a new object to existing tag "
<< "SiStripFedCablingRcd"
<< " in since mode " << std::endl;
dbservice->appendSinceTime<SiStripFedCabling>(obj.release(), Time_, "SiStripFedCablingRcd");
dbservice->appendSinceTime<SiStripFedCabling>(*obj, Time_, "SiStripFedCablingRcd");
}
} else {
edm::LogError("SiStripFedCablingManipulator") << "Service is unavailable" << std::endl;
Expand Down
2 changes: 0 additions & 2 deletions Calibration/LumiAlCaRecoProducers/plugins/CorrPCCProducer.cc
View file
Open in desktop
Expand Up @@ -580,8 +580,6 @@ void CorrPCCProducer::dqmEndRunProduce(edm::Run const& runSeg, const edm::EventS
throw std::runtime_error("PoolDBService required.");
}

delete pccCorrections;
Copy link
Contributor

Choose a reason for hiding this comment

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

This deletion can (must) be avoided here because writeOne is used in its "deprecated" form: correct?
Shouldn't you try to move to the "non deprecated" form with the reference as argument, instead of the pointer?

Copy link
Contributor Author

@ggovi ggovi Aug 31, 2021 •
edited

Choose a reason for hiding this comment

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

@perrotta The idea was to minimise the changes in this PR, and leave to the various code authors the migration to the new interface. Here we clearly had to options: 1. Remove the delete and leave the old interface 2. Leave the delete and move to new (reference-based) interface. I think 2 would not be fully satisfactory since we would be left with the use of pointers that has no justification. So, we should also remove the use of pointers.


histoFile->cd();
corrlumiAvg_h->Write();
scaleFactorAvg_h->Write();
Expand Down
3 changes: 1 addition & 2 deletions CommonTools/ConditionDBWriter/interface/ConditionDBWriter.h
View file
Open in desktop
Expand Up @@ -332,8 +332,7 @@ class ConditionDBWriter : public edm::EDAnalyzer {

edm::LogInfo("ConditionDBWriter") << "appending a new object to tag " << Record_ << " in since mode " << std::endl;

// The Framework will take control over the DB object now, therefore the release.
mydbservice->writeOne<T>(objPointer.release(), since, Record_);
mydbservice->writeOne<T>(*objPointer, since, Record_);
}

void setTime() {
Expand Down
11 changes: 10 additions & 1 deletion CondCore/DBOutputService/interface/OnlineDBOutputService.h
View file
Open in desktop
Expand Up @@ -37,7 +37,7 @@ namespace cond {

//
template <typename PayloadType>
bool writeForNextLumisection(const PayloadType* payload, const std::string& recordName) {
bool writeForNextLumisection(const PayloadType& payload, const std::string& recordName) {
cond::Time_t targetTime = getLastLumiProcessed() + m_latencyInLumisections;
auto t0 = std::chrono::high_resolution_clock::now();
logger().logInfo() << "Updating lumisection " << targetTime;
Expand Down Expand Up @@ -73,6 +73,15 @@ namespace cond {
return ret;
}

//
template <typename PayloadType>
bool writeForNextLumisection(const PayloadType* payloadPtr, const std::string& recordName) {
if (!payloadPtr)
throwException("Provided payload pointer is invalid.", "OnlineDBOutputService::writeForNextLumisection");
std::unique_ptr<const PayloadType> payload(payloadPtr);
return writeForNextLumisection<PayloadType>(*payload, recordName);
}

private:
cond::Time_t getLastLumiProcessed();

Expand Down
106 changes: 88 additions & 18 deletions CondCore/DBOutputService/interface/PoolDBOutputService.h
View file
Open in desktop
Expand Up @@ -58,11 +58,8 @@ namespace cond {
std::string tag(const std::string& recordName);
bool isNewTagRequest(const std::string& recordName);

//
template <typename T>
Hash writeOne(const T* payload, Time_t time, const std::string& recordName) {
if (!payload)
throwException("Provided payload pointer is invalid.", "PoolDBOutputService::writeOne");
Hash writeOne(const T& payload, Time_t time, const std::string& recordName) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
doStartTransaction();
cond::persistency::TransactionScope scope(m_session.transaction());
Expand All @@ -87,7 +84,7 @@ namespace cond {
return thePayloadHash;
}
}
thePayloadHash = m_session.storePayload(*payload);
thePayloadHash = m_session.storePayload(payload);
std::string payloadType = cond::demangledName(typeid(T));
if (newTag) {
createNewIOV(thePayloadHash, payloadType, time, myrecord);
Expand All @@ -108,18 +105,76 @@ namespace cond {
return thePayloadHash;
}

// warning: takes over the ownership of pointer "payload". Deprecated. Please move to the above referece-based function
template <typename T>
Hash writeOne(const T* payloadPtr, Time_t time, const std::string& recordName) {
if (!payloadPtr)
throwException("Provided payload pointer is invalid.", "PoolDBOutputService::writeOne");
std::unique_ptr<const T> payload(payloadPtr);
std::lock_guard<std::recursive_mutex> lock(m_mutex);
return writeOne<T>(*payload, time, recordName);
}

template <typename T>
void writeMany(const std::map<Time_t, std::shared_ptr<T> >& iovAndPayloads, const std::string& recordName) {
if (iovAndPayloads.empty())
return;
std::lock_guard<std::recursive_mutex> lock(m_mutex);
doStartTransaction();
cond::persistency::TransactionScope scope(m_session.transaction());
try {
this->initDB();
Record& myrecord = this->lookUpRecord(recordName);
m_logger.logInfo() << "Tag mapped to record " << recordName << ": " << myrecord.m_tag;
bool newTag = isNewTagRequest(recordName);
cond::Time_t lastSince;
cond::persistency::IOVEditor editor;
if (newTag) {
std::string payloadType = cond::demangledName(typeid(T));
editor = m_session.createIov(payloadType, myrecord.m_tag, myrecord.m_timetype, cond::SYNCH_ANY);
editor.setDescription("New Tag");
} else {
editor = m_session.editIov(myrecord.m_tag);
if (myrecord.m_onlyAppendUpdatePolicy) {
cond::TagInfo_t tInfo;
this->getTagInfo(myrecord.m_idName, tInfo);
lastSince = tInfo.lastInterval.since;
if (lastSince == cond::time::MAX_VAL)
lastSince = 0;
}
}
for (auto& iovEntry : iovAndPayloads) {
Time_t time = iovEntry.first;
auto payload = iovEntry.second;
if (myrecord.m_onlyAppendUpdatePolicy && !newTag) {
if (time <= lastSince) {
m_logger.logInfo() << "Won't append iov with since " << std::to_string(time)
<< ", because is less or equal to last available since = " << lastSince;
continue;
}
}
auto payloadHash = m_session.storePayload(*payload);
editor.insert(time, payloadHash);
}
cond::UserLogInfo a = this->lookUpUserLogInfo(myrecord.m_idName);
editor.flush(a.usertext);
if (m_autoCommit) {
doCommitTransaction();
}
} catch (const std::exception& er) {
cond::throwException(std::string(er.what()), "PoolDBOutputService::writeMany");
}
scope.close();
return;
}

// close the IOVSequence setting lastTill
void closeIOV(Time_t lastTill, const std::string& recordName);

// this one we need to avoid to adapt client code around... to be removed in the long term!
template <typename T>
void createNewIOV(const T* firstPayloadObj,
cond::Time_t firstSinceTime,
cond::Time_t,
const std::string& recordName) {
if (!firstPayloadObj)
throwException("Provided payload pointer is invalid.", "PoolDBOutputService::createNewIOV");
void createNewIOV(const T& payload, cond::Time_t firstSinceTime, const std::string& recordName) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);

Record& myrecord = this->lookUpRecord(recordName);
if (!myrecord.m_isNewTag) {
cond::throwException(myrecord.m_tag + " is not a new tag", "PoolDBOutputService::createNewIOV");
Expand All @@ -128,19 +183,25 @@ namespace cond {
cond::persistency::TransactionScope scope(m_session.transaction());
try {
this->initDB();
Hash payloadId = m_session.storePayload(*firstPayloadObj);
Hash payloadId = m_session.storePayload(payload);
createNewIOV(payloadId, cond::demangledName(typeid(T)), firstSinceTime, myrecord);
} catch (const std::exception& er) {
cond::throwException(std::string(er.what()), "PoolDBOutputService::createNewIov");
}
scope.close();
}

//
// warning: takes over the ownership of pointer "payload" - deprecated. Please move to the above reference-based function
template <typename T>
void appendSinceTime(const T* payloadObj, cond::Time_t sinceTime, const std::string& recordName) {
if (!payloadObj)
throwException("Provided payload pointer is invalid.", "PoolDBOutputService::appendSinceTime");
void createNewIOV(const T* payloadPtr, cond::Time_t firstSinceTime, cond::Time_t, const std::string& recordName) {
if (!payloadPtr)
throwException("Provided payload pointer is invalid.", "PoolDBOutputService::createNewIOV");
std::unique_ptr<const T> payload(payloadPtr);
this->createNewIOV<T>(*payload, firstSinceTime, recordName);
}

template <typename T>
void appendSinceTime(const T& payload, cond::Time_t sinceTime, const std::string& recordName) {
std::lock_guard<std::recursive_mutex> lock(m_mutex);
Record& myrecord = this->lookUpRecord(recordName);
if (myrecord.m_isNewTag) {
Expand All @@ -150,13 +211,22 @@ namespace cond {
doStartTransaction();
cond::persistency::TransactionScope scope(m_session.transaction());
try {
appendSinceTime(m_session.storePayload(*payloadObj), sinceTime, myrecord);
appendSinceTime(m_session.storePayload(payload), sinceTime, myrecord);
} catch (const std::exception& er) {
cond::throwException(std::string(er.what()), "PoolDBOutputService::appendSinceTime");
}
scope.close();
}

// warning: takes over the ownership of pointer "payload" - deprecated. Please move to the above reference-based function
template <typename T>
void appendSinceTime(const T* payloadPtr, cond::Time_t sinceTime, const std::string& recordName) {
if (!payloadPtr)
throwException("Provided payload pointer is invalid.", "PoolDBOutputService::appendSinceTime");
std::unique_ptr<const T> payload(payloadPtr);
this->appendSinceTime<T>(*payload, sinceTime, recordName);
}

void createNewIOV(const std::string& firstPayloadId, cond::Time_t firstSinceTime, const std::string& recordName);

bool appendSinceTime(const std::string& payloadId, cond::Time_t sinceTime, const std::string& recordName);
Expand Down
2 changes: 0 additions & 2 deletions CondCore/DBOutputService/src/PoolDBOutputService.cc
View file
Open in desktop
Expand Up @@ -287,10 +287,8 @@ bool cond::service::PoolDBOutputService::appendSinceTime(const std::string& payl
cond::Time_t time,
Record& myrecord) {
m_logger.logInfo() << "Updating existing tag " << myrecord.m_tag << ", adding iov with since " << time;
std::string payloadType("");
try {
cond::persistency::IOVEditor editor = m_session.editIov(myrecord.m_tag);
payloadType = editor.payloadType();
editor.insert(time, payloadId);
cond::UserLogInfo a = this->lookUpUserLogInfo(myrecord.m_idName);
editor.flush(a.usertext);
Expand Down