[RF] Make RooProdPdf::_pdfNSetList a std::vector and not RooLinkedList #8595
Conversation
guitargeek
force-pushed
the
RooProdPdf_1
branch
from
7776c5a to
d211342
Compare
roofit/roofitcore/inc/LinkDef.h
Outdated
| #pragma read sourceClass="RooProdPdf" targetClass="RooProdPdf" version="[4]" \ | ||
| source="RooLinkedList _pdfNSetList" target="_pdfNSetList" \ | ||
| code="{RooFIter iter = onfile._pdfNSetList.fwdIterator(); \ | ||
| while(auto nset = (RooArgSet*)iter.next()) { \ | ||
| _pdfNSetList.emplace_back(*nset); \ | ||
| } \ | ||
| }"; |
There was a problem hiding this comment.
This could potentially leak. It's worth checking at least: You're creating a copy of the objects pointed to by the linked list. When the I/O operation is finished, the original objects are still around, and I'm not sure that anybody owns them. I hope that the linked list does, and will delete them once everything is deserialised, but I'm not sure.
At the same time, you cannot destroy them here in this loop, since they might be (co-)owned by another entity in the same file. Did you check what happens?
There was a problem hiding this comment.
Very good point, thanks for noticing! I solved this problem now by avoiding the copy. The RooLinkedList is now replaced with a std::vector<std::unique_ptr<RooArgSet>> instead of a std::vector<RooArgSet>. So the RooArgSet is not copied anymore, the ownership is only transferred from the RooLinkedList to the unique_ptr.
Fortunately we don't need to worry about co-owning, because as you also noticed from the RooProdPdf constructor the RooProdPdf object is responsible to call Delete() on the list.
There was a problem hiding this comment.
I think this is doing the right thing now. I don't think it's the case (since the member is private), but just be aware that there might be a file somewhere in the world where two objects see those pointers. Since you don't touch the pointer during the I/O operation, they will still have "shared ownership" after the I/O operation completes, but from then, the ProdPdf owns exclusively (via the unique_ptr), while the other object just observes.
That's probably all fine, because people observing the internal norm sets are asking for trouble. You could only achieve this via friend classes or other evil things.
| @@ -439,7 +431,6 @@ void RooProdPdf::initializeFromCmdArgList(const RooArgSet& fullPdfSet, const Roo | |||
|
|
|||
| RooProdPdf::~RooProdPdf() | |||
| { | |||
| _pdfNSetList.Delete() ; | |||
There was a problem hiding this comment.
This is the indication that the list on disc doesn't own the objects ...
There was a problem hiding this comment.
Right, that's a good hint!
guitargeek
force-pushed
the
RooProdPdf_1
branch
2 times, most recently
from
c3a8a00 to
acab81d
Compare
|
Starting build on |
|
Build failed on mac11/cxx17. |
Part of the RooProdPdf modernization with the intention of eventually deprecating the RooLinkedList. This change reduces the size of a RooProdPdf from 1600 to 1496 bytes. A manual schema evolution rule to test the reading of older RooProdPdfs is implemented in the `LinkDef.h`. The schma evolution is tested by the stressRooFit tests, which are reading some RooProdPdfs from the reference files.
guitargeek
force-pushed
the
RooProdPdf_1
branch
from
acab81d to
78cade8
Compare
|
Starting build on |
|
Build failed on mac11/cxx17. |
|
Build failed on ROOT-ubuntu2004/soversion. Errors:
|
Part of the RooProdPdf modernization with the intention of eventually
deprecating the RooLinkedList.
This change reduces the size of a RooProdPdf from 1600 to 1496 bytes.
A manual schema evolution rule to test the reading of older RooProdPdfs
is implemented in the
LinkDef.h. The schma evolution is tested bythe stressRooFit tests, which are reading some RooProdPdfs from the
reference files.