-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Make branch description non mutable redux #86
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,7 +107,7 @@ namespace edm { | |
} | ||
|
||
void | ||
BranchDescription::initBranchName() const { | ||
BranchDescription::initBranchName() { | ||
if(!branchName().empty()) { | ||
return; // already called | ||
} | ||
|
@@ -155,7 +155,7 @@ namespace edm { | |
} | ||
|
||
void | ||
BranchDescription::initFromDictionary() const { | ||
BranchDescription::initFromDictionary() { | ||
if(bool(wrappedType())) { | ||
return; // already initialized; | ||
} | ||
|
@@ -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()) { | ||
|
@@ -366,12 +368,6 @@ namespace edm { | |
|
||
WrapperInterfaceBase const* | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
@@ -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_) { | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is correct.