-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add an (Open Source) license? #2
Comments
This is an issue that has not been discussed and decided on at a collaboration level. (i.e. the specific license we should use) Some of the software here will be largely copied from the Juggler base, but then modified to fit the new framework. That may require copying the license used for the original code. I'm happy to do whatever, but it needs to be endorsed by the community. I agree that we should probably resolve this within the 1-2 weeks. |
I want to point out that https://github.com/eic/EICrecon/blob/main/RECON/plugins/BarrelEMCal/JFactory_EcalBarrelRawCalorimeterHit.cc is clearly inspired by LGPL code (including copied code segments) and that you are getting close to violation territory, if you are not already in it. With Juggler we went through this decision process and decided on LGPL. I would ask you to respect this decision and follow the requirements of the LGPL if you will be copying code and making it available in this repository. That does indeed mean licensing (at least those parts) under LGPL. |
…tead of std::vector This fixes memory leaks like: ``` Direct leak of 407056 byte(s) in 50882 object(s) allocated from: #0 0x55e48bee007d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 5069f7d2185cef71541ac1e93e2f6643618de2c4) #1 0x7fa316881fc5 in CalorimeterHitDigi::signal_sum_digi() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:254:23 #2 0x7fa3168807eb in CalorimeterHitDigi::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:127:9 #3 0x7fa3150b2e0c in RawCalorimeterHit_factory_LFHCALRawHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/FHCAL/RawCalorimeterHit_factory_LFHCALRawHits.h:86:9 #4 0x7fa315a0fcc1 in eicrecon::JFactoryPodioT<edm4hep::RawCalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13 #5 0x7fa315a4e8a8 in JFactoryT<edm4hep::RawCalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13 #6 0x7fa315a4d87b in std::vector<edm4hep::RawCalorimeterHit const*, std::allocator<edm4hep::RawCalorimeterHit const*> > JEvent::Get<edm4hep::RawCalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27 ```
…tead of std::vector This fixes memory leaks like: Direct leak of 389136 byte(s) in 48642 object(s) allocated from: #0 0x562ee9bce07d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 32bcf2da69e5409b22003a41cdb3023b1ebdbb21) #1 0x7fe7e2189715 in CalorimeterHitReco::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitReco.cc:240:20 #2 0x7fe7e114a70c in CalorimeterHit_factory_EcalBarrelScFiRecHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelScFiRecHits.h:88:9 #3 0x7fe7e133f8a1 in eicrecon::JFactoryPodioT<edm4eic::CalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13 #4 0x7fe7e1352148 in JFactoryT<edm4eic::CalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13 #5 0x7fe7e135096b in std::vector<edm4eic::CalorimeterHit const*, std::allocator<edm4eic::CalorimeterHit const*> > JEvent::Get<edm4eic::CalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
…ad of std::vector This fixes a memory leak: Direct leak of 640736 byte(s) in 80092 object(s) allocated from: #0 0x562ee9bce07d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 32bcf2da69e5409b22003a41cdb3023b1ebdbb21) #1 0x7fe7e11690b2 in ImagingPixelReco::execute() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/ImagingPixelReco.h:133:32 #2 0x7fe7e11674e9 in CalorimeterHit_factory_EcalBarrelImagingRecHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelImagingRecHits.h:65:9 #3 0x7fe7e133f8a1 in eicrecon::JFactoryPodioT<edm4eic::CalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13 #4 0x7fe7e1352148 in JFactoryT<edm4eic::CalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13 #5 0x7fe7e135096b in std::vector<edm4eic::CalorimeterHit const*, std::allocator<edm4eic::CalorimeterHit const*> > JEvent::Get<edm4eic::CalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
…#741) This fixes memory leaks like: ``` Direct leak of 407056 byte(s) in 50882 object(s) allocated from: #0 0x55e48bee007d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 5069f7d2185cef71541ac1e93e2f6643618de2c4) #1 0x7fa316881fc5 in CalorimeterHitDigi::signal_sum_digi() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:254:23 #2 0x7fa3168807eb in CalorimeterHitDigi::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:127:9 #3 0x7fa3150b2e0c in RawCalorimeterHit_factory_LFHCALRawHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/FHCAL/RawCalorimeterHit_factory_LFHCALRawHits.h:86:9 #4 0x7fa315a0fcc1 in eicrecon::JFactoryPodioT<edm4hep::RawCalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13 #5 0x7fa315a4e8a8 in JFactoryT<edm4hep::RawCalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13 #6 0x7fa315a4d87b in std::vector<edm4hep::RawCalorimeterHit const*, std::allocator<edm4hep::RawCalorimeterHit const*> > JEvent::Get<edm4hep::RawCalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27 ``` Leaks were discovered by LSAN that was enabled on CI recently #514. Alternative way is to free the memory manually #731, or fix #742.
### Briefly, what does this PR introduce? This clarifies that the algorithms (and arguably everything else here) was always LGPL as per the upstream license conditions of juggler. ### What kind of change does this PR introduce? - [x] Bug fix (issue #2, in part) - [ ] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No. ### Does this PR change default behavior? No.
### Briefly, what does this PR introduce? Fixes overlow when running `eicrecon -c`, `--configs` (display configuration parameters). For small `max_key_length`, `max_key_length-4` wraps around. ``` Configuration Parameters: terminate called after throwing an instance of 'std::length_error' what(): basic_string::_M_create Program received signal SIGABRT, Aborted. 0x00007ffff7aacd3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6 (gdb) bt #0 0x00007ffff7aacd3c in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #1 0x00007ffff7a5df32 in raise () from /lib/x86_64-linux-gnu/libc.so.6 #2 0x00007ffff7a48472 in abort () from /lib/x86_64-linux-gnu/libc.so.6 #3 0x00007ffff7cc2919 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6 #4 0x00007ffff7ccde1a in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6 #5 0x00007ffff7ccde85 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6 #6 0x00007ffff7cce0d8 in __cxa_throw () from /lib/x86_64-linux-gnu/libstdc++.so.6 #7 0x00007ffff7cc51e9 in std::__throw_length_error(char const*) () from /lib/x86_64-linux-gnu/libstdc++.so.6 #8 0x00007ffff7d648b9 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long) () from /lib/x86_64-linux-gnu/libstdc++.so.6 #9 0x00007ffff7d6495c in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct(unsigned long, char) () from /lib/x86_64-linux-gnu/libstdc++.so.6 #10 0x0000555555564476 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::allocator<char> > (this=0x7fffffff6610, __n=18446744073709551615, __c=32 ' ', __a=...) at /usr/include/c++/12/bits/basic_string.h:659 #11 0x0000555555560b3e in jana::PrintConfigParameters (app=0x55555560e000) at /home/wdconinc/git/EICrecon/.worktree/ci-mt/src/utilities/eicrecon/eicrecon_cli.cpp:328 #12 0x00005555555611fa in jana::Execute (app=0x55555560e000, options=...) at /home/wdconinc/git/EICrecon/.worktree/ci-mt/src/utilities/eicrecon/eicrecon_cli.cpp:362 #13 0x000055555555b94f in main (narg=4, argv=0x7fffffff6ac8) at /home/wdconinc/git/EICrecon/.worktree/ci-mt/src/utilities/eicrecon/eicrecon.cc:60 ``` ### What kind of change does this PR introduce? - [x] Bug fix (issue #__) - [ ] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No. ### Does this PR change default behavior? No. --------- Co-authored-by: Dmitry Kalinkin <[email protected]>
### Briefly, what does this PR introduce? A memory leak was caused by creating a new pointer `cl`, then push_back `*cl`, but not deleting the pointer. This modifies the algorithm to use objects instead of pointers. RVO and podio object encapsulation should ensure performance is not affected. Leak sanitizer output resolved: ``` Direct leak of 108512 byte(s) in 13564 object(s) allocated from: #0 0x55cbc7e9fded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 4702f11c9afe7b8bff422367fb6901ebdee7ea40) #1 0x7f92f7afe251 in eicrecon::CalorimeterClusterRecoCoG::reconstruct(edm4eic::ProtoCluster const&) const /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:293:10 #2 0x7f92f7af9dc9 in eicrecon::CalorimeterClusterRecoCoG::process(edm4eic::ProtoClusterCollection const*, edm4hep::SimCalorimeterHitCollection const*) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:55:18 #3 0x7f92f7cb9713 in eicrecon::CalorimeterClusterRecoCoG_factoryT::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h:73:48 #4 0x7f92f7cc0e54 in JMultifactoryHelperPodio<edm4eic::MCRecoClusterParticleAssociation>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20 #5 0x7f92ffc7ccf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3) Direct leak of 45664 byte(s) in 5708 object(s) allocated from: #0 0x55cbc7e9fded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 4702f11c9afe7b8bff422367fb6901ebdee7ea40) #1 0x7f92f7afe251 in eicrecon::CalorimeterClusterRecoCoG::reconstruct(edm4eic::ProtoCluster const&) const /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:293:10 #2 0x7f92f7af9dc9 in eicrecon::CalorimeterClusterRecoCoG::process(edm4eic::ProtoClusterCollection const*, edm4hep::SimCalorimeterHitCollection const*) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:55:18 #3 0x7f92f7cb9713 in eicrecon::CalorimeterClusterRecoCoG_factoryT::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h:73:48 #4 0x7f92f7cbac34 in JMultifactoryHelperPodio<edm4eic::Cluster>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20 #5 0x7f92ffc7ccf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3) ``` ### What kind of change does this PR introduce? - [x] Bug fix (issue #__) - [ ] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No. ### Does this PR change default behavior? No. Co-authored-by: Dmitry Kalinkin <[email protected]>
### Briefly, what does this PR introduce? When there are no track tips, we should delete the ActsExamples::Trajectories before moving on to the next one. Fixes: ``` Direct leak of 128928 byte(s) in 1343 object(s) allocated from: #0 0x556fe8256ded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 7089f1e0b4fe65f89c79d38a99df5a0d1554ba34) #1 0x7f0f0594cb83 in eicrecon::CKFTracking::process(boost::container::flat_multiset<std::reference_wrapper<ActsExamples::IndexSourceLink const>, ActsExamples::detail::CompareGeometryId, void> const&, std::vector<std::variant<Acts::Measurement<Acts::BoundIndices, 1ul>, Acts::Measurement<Acts::BoundIndices, 2ul>, Acts::Measurement<Acts::BoundIndices, 3ul>, Acts::Measurement<Acts::BoundIndices, 4ul>, Acts::Measurement<Acts::BoundIndices, 5ul>, Acts::Measurement<Acts::BoundIndices, 6ul> >, std::allocator<std::variant<Acts::Measurement<Acts::BoundIndices, 1ul>, Acts::Measurement<Acts::BoundIndices, 2ul>, Acts::Measurement<Acts::BoundIndices, 3ul>, Acts::Measurement<Acts::BoundIndices, 4ul>, Acts::Measurement<Acts::BoundIndices, 5ul>, Acts::Measurement<Acts::BoundIndices, 6ul> > > > const&, edm4eic::TrackParametersCollection const&) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/tracking/CKFTracking.cc:170:41 #2 0x7f0f0514f9c3 in eicrecon::CKFTracking_factory::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/global/tracking/CKFTracking_factory.cc:74:84 #3 0x7f0f0524d104 in JMultifactoryHelperPodio<edm4eic::TrackParameters>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20 #4 0x7f0f0cec0cf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3) ``` ### What kind of change does this PR introduce? - [x] Bug fix (issue: memory leak) - [ ] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No. ### Does this PR change default behavior? No. Co-authored-by: Dmitry Kalinkin <[email protected]>
The project lacks COPYING/LICENSE file.
The text was updated successfully, but these errors were encountered: