Fix Xrootd thread-safety issue in quality metric factory #13085
Conversation
Laboriously emulate from XrdCl the cases where we own the "response" pointer and those where we do not. I suspect this may have been leaking a few bytes per file open - not going to kill the memory budget, but likely good for those using valgrind!
|
A new Pull Request was created by @bbockelm (Brian Bockelman) for CMSSW_8_0_X. It involves the following packages: Utilities/XrdAdaptor @cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
|
Please test |
|
The tests are being triggered in jenkins. |
| @@ -66,7 +66,7 @@ friend class Source; | |||
|
|
|||
| static QualityMetricFactory *m_instance; | |||
|
|
|||
| typedef std::unordered_map<std::string, QualityMetricUniqueSource*> MetricMap; | |||
| typedef tbb::concurrent_hash_map<std::string, QualityMetricUniqueSource*> MetricMap; | |||
There was a problem hiding this comment.
I think it would be better to use tbb::concurrent_unordered_map<std::string, std::shared_ptr<QualityMetricUniqueSource>> as the type. That type is guaranteed to take no visible locks. std::concurrent_hash_map is preferred if one need concurrent erasure, which is not the case here.
| @@ -157,16 +157,17 @@ QualityMetricFactory * QualityMetricFactory::m_instance = new QualityMetricFacto | |||
| std::unique_ptr<QualityMetricSource> | |||
| QualityMetricFactory::get(timespec now, const std::string &id) | |||
| { | |||
| MetricMap::const_iterator it = m_instance->m_sources.find(id); | |||
There was a problem hiding this comment.
If we switched to concurrent_unordered_map than I believe the safe way to do this is
auto itFound = m_instance->m_sources.find(id);
if( itFound == m_instance->m_sources.end() ) {
//try to make a new one
auto source = std::make_unique<QualityMetricUniqueSource>(now);
auto insertResult = m_instance->m_sources.insert(std::make_pair(id, source.get());
itFound = insertResult.first;
if(insertResult.second) {
//this thread successfully inserted
source.release();
}
}
return itFound->second->newSource(now);There was a problem hiding this comment.
Ah - I struggled to find an explanation of the differences and performance considerations between concurrent_unordered_map and concurrent_hash_map. Semantically, either should work in this case. What's the advantage of the above?
There was a problem hiding this comment.
Nevermind - just saw your comment above.
cmsbuild
added
tests-pending
comparison-pending
and removed
comparison-available
tests-approved
labels
|
Pull request #13085 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please check and sign again. |
…_qmf_thread_safety
|
Pull request #13085 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @davidlange6 can you please check and sign again. |
|
@davidlange6 - sorry, there were some issues from developing both Xrootd patches in parallel. Merges fine now. |
|
please test |
|
+1 |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
The tests are being triggered in jenkins. |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs after it passes the integration tests. This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar |
|
-1 Tested at: 556e64e ---> test testRecoMETMETProducers had ERRORS you can see the results of the tests here: The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
Fix Xrootd thread-safety issue in quality metric factory
The quality metric factory did not safely access its
m_sourcesmap. This never caused problems before as this is only done on file open and we didn't have many concurrent file opens.Fixes (I hope) issue #13081.
As a bonus, I did a small commit that handles memory management for the
responseparameter to theHandleResponsevirtual function in the same manner as the upstreamXrdCl. That is, I delete for callbacks where they delete; I leaveresponseuntouched for operations where they leave it untouched.