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

Make branch description non mutable redux #86

Merged
merged 2 commits into from Jul 15, 2013
Merged

Make branch description non mutable redux #86

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f677f01
Make BranchDescription not mutable
wmtan Jul 10, 2013
c16a3d9
Resolve merge conflict
wmtan Jul 11, 2013
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
66 changes: 41 additions & 25 deletions DataFormats/Provenance/interface/BranchDescription.h
View file
Open in desktop
Expand Up @@ -58,14 +58,14 @@ namespace edm {

~BranchDescription() {}

void init() const {
void init() {
initBranchName();
initFromDictionary();
}

void initBranchName() const;
void initBranchName();

void initFromDictionary() const;
void initFromDictionary();

void write(std::ostream& os) const;

Expand All @@ -81,42 +81,58 @@ namespace edm {
std::string const& className() const {return fullClassName();}
std::string const& friendlyClassName() const {return friendlyClassName_;}
std::string const& productInstanceName() const {return productInstanceName_;}
bool& produced() const {return transient_.produced_;}
bool produced() const {return transient_.produced_;}
bool& produced() {return transient_.produced_;}
bool present() const {return !transient_.dropped_;}
bool& dropped() const {return transient_.dropped_;}
bool& onDemand() const {return transient_.onDemand_;}
bool& transient() const {return transient_.transient_;}
TypeWithDict& wrappedType() const {return transient_.wrappedType_;}
TypeWithDict& unwrappedType() const {return transient_.unwrappedType_;}
bool dropped() const {return transient_.dropped_;}
bool& dropped() {return transient_.dropped_;}
bool onDemand() const {return transient_.onDemand_;}
bool& onDemand() {return transient_.onDemand_;}
bool transient() const {return transient_.transient_;}
bool& transient() {return transient_.transient_;}
TypeWithDict const& wrappedType() const {return transient_.wrappedType_;}
TypeWithDict& wrappedType() {return transient_.wrappedType_;}
TypeWithDict const& unwrappedType() const {return transient_.unwrappedType_;}
TypeWithDict& unwrappedType() {return transient_.unwrappedType_;}
TypeID wrappedTypeID() const {return TypeID(transient_.wrappedType_.typeInfo());}
TypeID unwrappedTypeID() const {return TypeID(transient_.unwrappedType_.typeInfo());}
int& splitLevel() const {return transient_.splitLevel_;}
int& basketSize() const {return transient_.basketSize_;}
int splitLevel() const {return transient_.splitLevel_;}
int& splitLevel() {return transient_.splitLevel_;}
int basketSize() const {return transient_.basketSize_;}
int& basketSize() {return transient_.basketSize_;}

ParameterSetID const& parameterSetID() const {return transient_.parameterSetID_;}
std::string const& moduleName() const {return transient_.moduleName_;}

std::map<ProcessConfigurationID, ParameterSetID>& parameterSetIDs() const {
std::map<ProcessConfigurationID, ParameterSetID> const& parameterSetIDs() const {
return transient_.parameterSetIDs_;
}
std::map<ProcessConfigurationID, std::string>& moduleNames() const {
std::map<ProcessConfigurationID, ParameterSetID>& parameterSetIDs() {
return transient_.parameterSetIDs_;
}
std::map<ProcessConfigurationID, std::string> const& moduleNames() const {
return transient_.moduleNames_;
}
std::map<ProcessConfigurationID, std::string>& moduleNames() {
return transient_.moduleNames_;
}
ParameterSetID const& psetID() const;
bool isPsetIDUnique() const {return parameterSetIDs().size() == 1;}
std::set<std::string> const& branchAliases() const {return branchAliases_;}
std::set<std::string>& branchAliases() {return branchAliases_;}
std::string& branchName() const {return transient_.branchName_;}
std::string const& branchName() const {return transient_.branchName_;}
std::string& branchName() {return transient_.branchName_;}
BranchType const& branchType() const {return branchType_;}
std::string& wrappedName() const {return transient_.wrappedName_;}
WrapperInterfaceBase*& wrapperInterfaceBase() const {return transient_.wrapperInterfaceBase_;}
std::string const& wrappedName() const {return transient_.wrappedName_;}
std::string& wrappedName() {return transient_.wrappedName_;}
WrapperInterfaceBase*& wrapperInterfaceBase() {return transient_.wrapperInterfaceBase_;}
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume you don't need a const version of wrapperInterfaceBase() because you already have getInterface() const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct.


WrapperInterfaceBase const* getInterface() const;
void setDropped() const {dropped() = true;}
void setOnDemand() const {onDemand() = true;}
void setDropped() {dropped() = true;}
void setOnDemand() {onDemand() = true;}
void updateFriendlyClassName();

void initializeTransients() const {transient_.reset();}
void initializeTransients() {transient_.reset();}

struct Transients {
Transients();
Expand Down Expand Up @@ -152,12 +168,12 @@ namespace edm {
// ID's of process configurations for products on this branch
// with corresponding parameter set IDs,
// This is initialized if and only if produced_ is false.
mutable std::map<ProcessConfigurationID, ParameterSetID> parameterSetIDs_;
std::map<ProcessConfigurationID, ParameterSetID> parameterSetIDs_;

// ID's of process configurations for products on this branch
// with corresponding module names
// This is initialized if and only if produced_ is false.
mutable std::map<ProcessConfigurationID, std::string> moduleNames_;
std::map<ProcessConfigurationID, std::string> moduleNames_;

// Is the class of the branch marked as transient
// in the data dictionary
Expand All @@ -170,7 +186,7 @@ namespace edm {
TypeWithDict unwrappedType_;

// A pointer to a polymorphic object to obtain typed Wrapper.
mutable WrapperInterfaceBase* wrapperInterfaceBase_;
WrapperInterfaceBase* wrapperInterfaceBase_;

// The split level of the branch, as marked
// in the data dictionary.
Expand All @@ -195,7 +211,7 @@ namespace edm {
std::string processName_;

// An ID uniquely identifying the branch
mutable BranchID branchID_;
BranchID branchID_;

// the full name of the type of product this is
std::string fullClassName_;
Expand All @@ -213,9 +229,9 @@ namespace edm {
// If this branch *is* an EDAlias, this field is the BranchID
// of the branch for which this branch is an alias.
// If this branch is not an EDAlias, the normal case, this field is 0.
mutable BranchID aliasForBranchID_;
BranchID aliasForBranchID_;

mutable Transients transient_;
Transients transient_;
};

inline
Expand Down
3 changes: 2 additions & 1 deletion DataFormats/Provenance/interface/ProductRegistry.h
View file
Open in desktop
Expand Up @@ -128,6 +128,8 @@ namespace edm {

void initializeTransients() {transient_.reset();}

bool frozen() const {return transient_.frozen_;}

struct Transients {
Transients();
void reset();
Expand Down Expand Up @@ -158,7 +160,6 @@ namespace edm {
transient_.anyProductProduced_ = true;
}

bool frozen() const {return transient_.frozen_;}
void freezeIt(bool frozen = true) {transient_.frozen_ = frozen;}

void updateConstProductRegistry();
Expand Down
14 changes: 5 additions & 9 deletions DataFormats/Provenance/src/BranchDescription.cc
View file
Open in desktop
Expand Up @@ -107,7 +107,7 @@ namespace edm {
}

void
BranchDescription::initBranchName() const {
BranchDescription::initBranchName() {
if(!branchName().empty()) {
return; // already called
}
Expand Down Expand Up @@ -155,7 +155,7 @@ namespace edm {
}

void
BranchDescription::initFromDictionary() const {
BranchDescription::initFromDictionary() {
if(bool(wrappedType())) {
return; // already initialized;
}
Expand All @@ -179,6 +179,8 @@ namespace edm {
basketSize() = invalidBasketSize;
return;
}
wrappedType().invokeByName(wrapperInterfaceBase(), "getInterface");
assert(wrapperInterfaceBase() != 0);
Reflex::PropertyList wp = Reflex::Type::ByTypeInfo(wrappedType().typeInfo()).Properties();
transient() = (wp.HasProperty("persistent") ? wp.PropertyAsString("persistent") == std::string("false") : false);
if(transient()) {
Expand Down Expand Up @@ -366,12 +368,6 @@ namespace edm {

WrapperInterfaceBase const*
Copy link
Contributor

Choose a reason for hiding this comment

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

If this really becomes a performance problem, we can make caching thread-safe. The real problem would be the call to TypeWithDict::byName(...) not being thread safe.

BranchDescription::getInterface() const {
if(wrapperInterfaceBase() == 0) {
// This could be done in init(), but we only want to do it on demand, for performance reasons.
TypeWithDict type = TypeWithDict::byName(wrappedName());
type.invokeByName(wrapperInterfaceBase(), "getInterface");
assert(wrapperInterfaceBase() != 0);
}
return wrapperInterfaceBase();
return transient_.wrapperInterfaceBase_;
}
}
2 changes: 0 additions & 2 deletions DataFormats/Streamer/interface/StreamedProducts.h
View file
Open in desktop
Expand Up @@ -49,7 +49,6 @@ namespace edm {
void allocateForReading();
void setNewClassType();
void clearClassType();
void initializeTransients();

void clear() {
prod_= 0;
Expand Down Expand Up @@ -92,7 +91,6 @@ namespace edm {
EventSelectionIDVector const& eventSelectionIDs() const {return eventSelectionIDs_;}
BranchListIndexes const& branchListIndexes() const {return branchListIndexes_;}
SendProds& products() {return products_;}
void initializeTransients();
private:
EventAuxiliary aux_;
ProcessHistory processHistory_;
Expand Down
14 changes: 1 addition & 13 deletions DataFormats/Streamer/src/StreamedProducts.cc
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ namespace edm {
std::vector<BranchID> const* parents) :
desc_(&desc), present_(present), parents_(parents), prod_(const_cast<void*>(prod)), classRef_() {
if(present_ && prod == 0) {
desc.init();
const_cast<BranchDescription&>(desc).init();
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be a thread safety issue? I don't know when StreamedProducts are constructed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this change was OK because the line of code is only executed immediately before throwing a fatal exception. If not, the code can be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

While that fatal exception is in the process of forming on one thread another thread could be reading that BranchDescription and the modification while reading could lead to a crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I'll fix this. It's easy to fix by making a local copy, which is not a performance issue before a fatal exception.

throw edm::Exception(edm::errors::LogicError, "StreamedProduct::StreamedProduct\n")
<< "A product with a status of 'present' is not actually present.\n"
<< "The branch name is " << desc.branchName() << "\n"
Expand All @@ -36,18 +36,6 @@ namespace edm {
}
}

void
StreamedProduct::initializeTransients() {
desc_->init();
}

void
SendEvent::initializeTransients() {
for(StreamedProduct& prod : products_) {
prod.initializeTransients();
}
}

void
SendJobHeader::initializeTransients() {
for(BranchDescription& desc : descs_) {
Expand Down
53 changes: 30 additions & 23 deletions FWCore/FWLite/src/BranchMapReader.cc
View file
Open in desktop
Expand Up @@ -102,7 +102,6 @@ namespace fwlite {
bReg = metaDataTree->GetBranch(edm::poolNames::productDescriptionBranchName().c_str());
bReg->SetAddress(ppReg);
bReg->GetEntry(0);
(*ppReg)->setFrozen(false);
}
return bReg;
}
Expand Down Expand Up @@ -168,17 +167,19 @@ namespace fwlite {
TBranch* br = getBranchRegistry(&pReg);

if(0 != br) {
edm::ProductRegistry::ProductList const& prodList = reg.productList();
edm::ProductRegistry::ProductList& prodList = reg.productListUpdator();

for(edm::ProductRegistry::ProductList::const_iterator it = prodList.begin(), itEnd = prodList.end(); it != itEnd; ++it) {
if(edm::InEvent == it->second.branchType()) {
for(auto& item : prodList) {
edm::BranchDescription& prod = item.second;
if(edm::InEvent == prod.branchType()) {
// call to regenerate branchName
it->second.init();
branchDescriptionMap_.insert(bidToDesc::value_type(it->second.branchID(), it->second));
prod.init();
branchDescriptionMap_.insert(bidToDesc::value_type(prod.branchID(), prod));
}
}
mapperFilled_ = true;
}
reg.setFrozen(false);
return 0 != br;
}

Expand Down Expand Up @@ -265,16 +266,18 @@ namespace fwlite {
TBranch *br = getBranchRegistry(&pReg);

if(0 != br) {
edm::ProductRegistry::ProductList const& prodList = reg.productList();
edm::ProductRegistry::ProductList& prodList = reg.productListUpdator();

for(edm::ProductRegistry::ProductList::const_iterator it = prodList.begin(), itEnd = prodList.end(); it != itEnd; ++it) {
if(edm::InEvent == it->second.branchType()) {
for(auto& item : prodList) {
edm::BranchDescription& prod = item.second;
if(edm::InEvent == prod.branchType()) {
// call to regenerate branchName
it->second.init();
branchDescriptionMap_.insert(bidToDesc::value_type(it->second.branchID(), it->second));
prod.init();
branchDescriptionMap_.insert(bidToDesc::value_type(prod.branchID(), prod));
}
}
}
reg.setFrozen(false);
return 0 != br;
}

Expand Down Expand Up @@ -375,17 +378,19 @@ namespace fwlite {
TBranch *br = getBranchRegistry(&pReg);

if(0 != br) {
edm::ProductRegistry::ProductList const& prodList = reg.productList();
edm::ProductRegistry::ProductList& prodList = reg.productListUpdator();

for(edm::ProductRegistry::ProductList::const_iterator it = prodList.begin(), itEnd = prodList.end(); it != itEnd; ++it) {
if(edm::InEvent == it->second.branchType()) {
for(auto& item : prodList) {
edm::BranchDescription& prod = item.second;
if(edm::InEvent == prod.branchType()) {
// call to regenerate branchName
it->second.init();
branchDescriptionMap_.insert(bidToDesc::value_type(it->second.branchID(), it->second));
// std::cout << "v11 updatefile " << it->second.branchID() << std::endl;
prod.init();
branchDescriptionMap_.insert(bidToDesc::value_type(prod.branchID(), prod));
// std::cout << "v11 updatefile " << prod.branchID() << std::endl;
}
}
}
reg.setFrozen(false);
return 0 != br;
}

Expand Down Expand Up @@ -488,17 +493,19 @@ namespace fwlite {
TBranch *br = getBranchRegistry(&pReg);

if(0 != br) {
edm::ProductRegistry::ProductList const& prodList = reg.productList();
edm::ProductRegistry::ProductList& prodList = reg.productListUpdator();

for(edm::ProductRegistry::ProductList::const_iterator it = prodList.begin(), itEnd = prodList.end(); it != itEnd; ++it) {
if(edm::InEvent == it->second.branchType()) {
for(auto& item : prodList) {
edm::BranchDescription& prod = item.second;
if(edm::InEvent == prod.branchType()) {
// call to regenerate branchName
it->second.init();
branchDescriptionMap_.insert(bidToDesc::value_type(it->second.branchID(), it->second));
// std::cout << "v11 updatefile " << it->second.branchID() << std::endl;
prod.init();
branchDescriptionMap_.insert(bidToDesc::value_type(prod.branchID(), prod));
// std::cout << "v11 updatefile " << prod.branchID() << std::endl;
}
}
}
reg.setFrozen(false);
return 0 != br;
}

Expand Down
15 changes: 8 additions & 7 deletions FWCore/Framework/src/Schedule.cc
View file
Open in desktop
Expand Up @@ -414,20 +414,21 @@ namespace edm {

loadMissingDictionaries();

preg.setFrozen();

for (auto c : all_output_communicators_) {
c->setEventSelectionInfo(outputModulePathPositions, preg.anyProductProduced());
c->selectProducts(preg);
}

// Sanity check: make sure nobody has added a worker after we've
// already relied on the WorkerManager being full.
assert (all_workers_count == allWorkers().size());

ProcessConfigurationRegistry::instance()->insertMapped(*processConfiguration);
branchIDListHelper.updateRegistries(preg);
fillProductRegistryTransients(*processConfiguration, preg);

preg.setFrozen();

for (auto c : all_output_communicators_) {
c->setEventSelectionInfo(outputModulePathPositions, preg.anyProductProduced());
c->selectProducts(preg);
}

} // Schedule::Schedule


Expand Down