testdockerwcsim / XTriggerApplication

Other
2 stars 3 forks source link

Rewrite DataOut & fix a big memory leak #60

Closed tdealtry closed 4 years ago

tdealtry commented 4 years ago

Note this requires the WCSim branch https://github.com/tdealtry/WCSim/tree/leak

tdealtry commented 4 years ago

One more thing... there's still a WCSimReader memory leak, but only when TChaining multiple files together. It doesn't leak with a single big file. I've not worked out why. If anyone has seen something like this before, please speak up! Otherwise, I think we ignore it for the purposes of this PR

brichards64 commented 4 years ago

Travis failed can you fix this bug?

UserTools/Factory/../DataOut/DataOut.cpp:380:18: error: 'class WCSimRootCherenkovDigiHit' has no member named 'SetPhotonIds'

tdealtry commented 4 years ago

This requires the WCSim branch https://github.com/tdealtry/WCSim/tree/leak

brichards64 commented 4 years ago

i see so should we put it on hold until that PR is merged into WCSim?

tdealtry commented 4 years ago

It's not worth merging yet, however we could get the code review finished and be ready for when WCSim/WCSim#286 is merged

ast0815 commented 4 years ago

The time handling is not correct, I think. At the very least it mixes TimeDelta and unit-less floating points in strange ways. I'll do a review to check what is going on.

ast0815 commented 4 years ago

Also when I run the current version of the code on my SNs, I get a segfault at these lines:

https://github.com/HKDAQ/TriggerApplication/blob/1871ea7085b00ba034a662f61adf817395a9b007/UserTools/DataOut/DataOut.cpp#L251-L253

tdealtry commented 4 years ago

Hopefully d320589 fixes the seg fault

ast0815 commented 4 years ago

The segfault seems to be gone now. 👍

ast0815 commented 4 years ago

The timing looks ok now. I'll test it with a supernova today or on Monday.

tdealtry commented 4 years ago

Great thanks @ast0815!