Thread safe changes for Particle class #1129
Conversation
|
A new Pull Request was created by @vkuznet (Valentin Kuznetsov) for CMSSW_7_0_X. Thread safe changes for Particle class It involves the following packages: DataFormats/Candidate @smuzaffar, @nclopezo, @vadler can you please review it and eventually sign? Thanks. |
|
@Dr15Jones please review |
| mutable bool cachePolarFixed_, cacheCartesianFixed_; | ||
| #endif | ||
| /// set internal cache | ||
| inline void cachePolar() const { |
There was a problem hiding this comment.
I believe this should be moved to the .cc file.
|
@ktf I'm waiting for @vkuznet 's reaction on @Dr15Jones 's comments. |
|
Again I will trust in @Dr15Jones 's expertise... ;-) |
|
@vkuznet This looks thread-safe to me, however since the default (*) form is being used it means you are forcing the CPU to synchronize all unrelated std::atomics when you do that call. Please change to the '.load()' and '.store()' form and use the more permissive 'std::memory_order_acquire' and 'std::memory_order_release' so the CPU only has to synchronize when the exact same memory location is changed. |
|
@vkuznet Valentin, we just added a new class |
|
-1 tmp/slc5_amd64_gcc481/src/DataFormats/HLTReco/src/DataFormatsHLTReco/a/xr.o: In function `(anonymous namespace)::method_17941(void*, void*, std::vector > const&, void*)': xr.cc:(.text+0x3471d): undefined reference to`reco::Particle::~Particle()' tmp/slc5_amd64_gcc481/src/DataFormats/HLTReco/src/DataFormatsHLTReco/a/xr.o: In function `trigger::TriggerObject::particle(int, ROOT::Math::PositionVector3D, ROOT::Math::DefaultCoordinateSystemTag> const&, int, bool) const': xr.cc:(.text._ZNK7trigger13TriggerObject8particleEiRKN4ROOT4Math16PositionVector3DINS2_11Cartesian3DIdEENS2_26DefaultCoordinateSystemTagEEEib[_ZNK7trigger13TriggerObject8particleEiRKN4ROOT4Math16PositionVector3DINS2_11Cartesian3DIdEENS2_26DefaultCoordinateSystemTagEEEib]+0xd5): undefined reference to`reco::Particle::Particle(int, ROOT::Math::LorentzVectorROOT::Math::PxPyPzE4D const&, ROOT::Math::PositionVector3DROOT::Math::Cartesian3D const&, int, int, bool)' >> Compiling /build/cmsbuild/jenkins-workarea/workspace/Pull-Request-Integration/ARCHITECTURE/slc5_amd64_gcc481/CMSSW_7_0_X_2013-11-07-0200/src/PhysicsTools/PatUtils/src/CaloIsolationEnergy.cc collect2: error: ld returned 1 exit status gmake: **\* [tmp/slc5_amd64_gcc481/src/DataFormats/HLTReco/src/DataFormatsHLTReco/libDataFormatsHLTReco.so] Error 1 gmake: **\* Waiting for unfinished jobs.... --->> genreflex: INFO: Parsing file src/PhysicsTools/PatUtils/src/classes.h with GCC_XML OK --->> genreflex: INFO: Generating Reflex Dictionary class pat::DiObjectProxy you can see the results of the tests here: |
|
Chris, I'll certainly look at this. On Nov 7, 2013, at ,Nov 7, 11:02 AM, Chris Jones wrote:
|
|
Even though I submitted update for load/store. I did not resolve yet this problem. So far I'm puzzled why reflex dictionary phase fails to see ctor/dtor's of this class. Chris, do you have any insight on this? On Nov 7, 2013, at ,Nov 7, 11:49 AM, cmsbuild wrote:
|
|
I was able to fix this issue by modifying HLTReco/BuildFile.xml and adding over there. It brings Candidate shared library which has definition of ctor/dtor's of Particle class in order to build HLTReco reflex dictionary. Please let me know if it is acceptable and I can add this change into this pull request. On Nov 7, 2013, at ,Nov 7, 11:49 AM, cmsbuild wrote:
|
|
That is the right thing to do. In fact, it is technically a requirement we have for all DataFormats packages which is they must explicitly link with every package they use. |
| /// set 4-momentum | ||
| void Particle::setP4( const LorentzVector & p4 ) { | ||
| *p4Cartesian_.load(std::memory_order_acquire) = p4; | ||
| *p4Polar_.load(std::memory_order_acquire) = p4; |
There was a problem hiding this comment.
p4Cartesian_ or p4Polar_ could be nullptr at this point which would cause a seg fault
|
Please note, that another class from this package (LeafCandidate.h) will require to undergo the same set of changes. But I wanted Particle class to get done and ensure that everything will run ok. |
|
-1 ---> test testDataFormatsCandidate had ERRORS you can see the results of the tests here: |
|
@cmsbuild could you please re-run usual tests. |
|
+1 |
|
-1 Error in one of my test workflows: Begin processing the 5th record. Run 1, Event 105, LumiSection 2 at 21-Nov-2013 09:04:13.954 CET A fatal system signal has occurred: segmentation violation #0 0x0000003d5d89a075 in waitpid () from /lib64/libc.so.6
|
|
-1 ---> test testDataFormatsCandidate had ERRORS you can see the results of the tests here: |
|
-1 Compilation:
|
|
-1 Now it compiles but I find the same runtime error as before: Begin processing the 5th record. Run 1, Event 105, LumiSection 2 at 22-Nov-2013 17:06:21.565 CET A fatal system signal has occurred: segmentation violation #0 0x0000003d5d89a075 in waitpid () from /lib64/libc.so.6 |
|
To reproduce: In CMSSW_7_0_X_2013-11-22-0200 I do: Go for a chat and a coffee....cd HLTrigger/Configuration/test/ Then execute: The crash occurs in the second step which reads in the just created output file: Presumably there is a persistency problem with the changes to the Particle class, as just using it |
|
Hi Martin, On Nov 22, 2013, at ,Nov 22, 11:40 AM, Martin Grunewald notifications@github.com wrote:
|
| <version ClassVersion="11" checksum="2698562380"/> | ||
| <version ClassVersion="10" checksum="2605909260"/> | ||
| </class> | ||
| <ioread sourceClass = "reco::Particle" version="[1-]" targetClass="reco::Particle" source = "" target="cachePolarFixed_"> | ||
| <![CDATA[cachePolarFixed_ = false;]]> | ||
| <ioread sourceClass = "reco::Particle" version="[1-]" targetClass="reco::Particle" source = "" target="p4Polar_"> |
There was a problem hiding this comment.
@pcanal Philippe, is this the correct iorule to deal with a transient pointer which is owned by the class?
|
Hi Valentin. I confirm the same findings of Martin. Andrea [1]
4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC Valentin Kuznetsov notifications@github.com ha scritto:
This message was sent using IMP, the Internet Messaging Program. |
|
-1 ---> test runtestTqafTopEventSelection had ERRORS you can see the results of the tests here: |
|
+1 |
|
+1 |
1 similar comment
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it? |
Multithreading fixes -- Thread safe changes for Particle class.
…v1.16 Updated correlator layer 1 e/gamma sorter
No description provided.