|
[Rivet-svn] r2035 - in trunk: . include/Rivet srcblackhole at projects.hepforge.org blackhole at projects.hepforge.orgSat Nov 7 01:05:40 GMT 2009
Author: buckley Date: Sat Nov 7 01:05:38 2009 New Revision: 2035 Log: ProjectionHandler system overhauled to do all its memory management and dependency tracking automatically via shared_ptrs, at the expense of much stress and brain cells. It's now a thing of beauty ;) As a result, Projections and Analyses can now check to eliminate duplicate proj-name typos and the memory space is both more efficient and finally devoid of undefined-behaviour-type pointers to deallocated memory (which we mostly got away with because new projections got allocated into the same space...) Modified: trunk/ChangeLog trunk/include/Rivet/ProjectionApplier.hh trunk/include/Rivet/ProjectionHandler.hh trunk/src/AnalysisLoader.cc trunk/src/ProjectionApplier.cc trunk/src/ProjectionHandler.cc trunk/src/Run.cc Modified: trunk/ChangeLog ============================================================================== --- trunk/ChangeLog Sat Nov 7 00:56:50 2009 (r2034) +++ trunk/ChangeLog Sat Nov 7 01:05:38 2009 (r2035) @@ -1,3 +1,16 @@ +2009-11-07 Andy Buckley <andy at insectnation.org> + + * Now that the ProjHandler isn't full of defunct pointers (which + tend to coincidentally point to *new* Projection pointers rather + than undefined memory, hence it wasn't noticed until recently!), + use of a duplicate projection name is now banned with a helpful + message at runtime. + + * (Huge) overhaul of ProjectionHandler system to use shared_ptr: + projections are now deleted much more efficiently, naturally + cleaning themselves out of the central repository as they go out + of scope. + 2009-11-06 Andy Buckley <andy at insectnation.org> * Adding Cmp<double> specialisation, using fuzzyEquals(). Modified: trunk/include/Rivet/ProjectionApplier.hh ============================================================================== --- trunk/include/Rivet/ProjectionApplier.hh Sat Nov 7 00:56:50 2009 (r2034) +++ trunk/include/Rivet/ProjectionApplier.hh Sat Nov 7 01:05:38 2009 (r2035) @@ -21,7 +21,7 @@ ProjectionApplier(); // Ensure that inheritance is possible. - virtual ~ProjectionApplier() { } + virtual ~ProjectionApplier(); public: @@ -108,15 +108,14 @@ /// up the internal type management. template <typename PROJ> const PROJ& addProjection(const PROJ& proj, const std::string& name) { - getLog() << Log::TRACE << "Cloning projection " << proj.name() << endl; - const Projection* newpproj = proj.clone(); - getLog() << Log::TRACE << "Cloned projection " << proj.name() << " at " << newpproj << endl; - const Projection* reg = getProjHandler().registerClonedProjection(*this, &proj, newpproj, name); - assert(reg); - if (reg != newpproj) delete newpproj; - return dynamic_cast<const PROJ&>(*reg); + const Projection& reg = _addProjection(proj, name); + return dynamic_cast<const PROJ&>(reg); } + + /// Untemplated function to do the work... + const Projection& _addProjection(const Projection& proj, const std::string& name); + //@} Modified: trunk/include/Rivet/ProjectionHandler.hh ============================================================================== --- trunk/include/Rivet/ProjectionHandler.hh Sat Nov 7 00:56:50 2009 (r2034) +++ trunk/include/Rivet/ProjectionHandler.hh Sat Nov 7 01:05:38 2009 (r2035) @@ -7,9 +7,12 @@ #include "Rivet/ProjectionApplier.fhh" #include "Rivet/Projection.fhh" - namespace Rivet { + + /// Typedef for Projection (smart) pointer + typedef shared_ptr<const Projection> ProjHandle; + // Forward declaration. class ProjectionApplier; @@ -36,6 +39,49 @@ /// the registering object and its local name for the registered projection. /// class ProjectionHandler { + public: + + /// ProjectionApplier's destructor needs to trigger cleaning up the proj handler repo + friend class ProjectionApplier; + + /// Typedef for a vector of Projection pointers. + typedef set<ProjHandle> ProjHandles; + + /// @brief Typedef for the structure used to contain named projections for a + /// particular containing Analysis or Projection. + typedef map<const string, ProjHandle> NamedProjs; + + /// Enum to specify depth of projection search. + enum ProjDepth { SHALLOW, DEEP }; + + + private: + + /// Structure used to map a containing Analysis or Projection to its set of + /// contained projections. + typedef map<const ProjectionApplier*, NamedProjs> NamedProjsMap; + + /// Core data member, associating a given containing class (via a + /// ProjectionApplier pointer) to its contained projections. + NamedProjsMap _namedprojs; + + /// Cache of {@link Projection}s for reverse lookup, to speed up registering + /// new projections as @c _namedprojs gets large. + ProjHandles _projs; + + + private: + + /// Private destructor means no inheritance from this class. + ~ProjectionHandler(); + + /// The assignment operator is hidden. + ProjectionHandler& operator=(const ProjectionHandler&); + + /// The copy constructor is hidden. + ProjectionHandler(const ProjectionHandler&); + + private: /// @name Construction. */ @@ -48,56 +94,63 @@ /// @todo Threading? static ProjectionHandler* _instance; - /// Declare that the @a new projection is a clone of @a old by - /// copying the set of {@a old}'s registered projections into an - /// equivalent set for {@a new}. - void declareClone(const Projection* oldproj, const Projection* newproj); public: + /// Singleton creation function static ProjectionHandler* create(); public: - /// @name Projection registration. */ + + /// @name Projection registration //@{ /// Attach and retrieve a projection as a reference. const Projection& registerProjection(const ProjectionApplier& parent, - const Projection& proj, const string& name); + const Projection& proj, + const string& name); /// Attach and retrieve a projection as a pointer. const Projection* registerProjection(const ProjectionApplier& parent, - const Projection* proj, const string& name) { - if (!proj) return 0; - return ®isterProjection(parent, *proj, name); - } - - /// Attach and retrieve a cloned projection as a reference. Calls - /// {@c declareClone(&oldproj, &newproj)} before registering @a newproj. - const Projection& registerClonedProjection(const ProjectionApplier& parent, - const Projection& oldproj, - const Projection& newproj, - const string& name); + const Projection* proj, + const string& name); + //@} + + + private: + + /// @name Projection registration internal helpers + //@{ + + /// Try to get an equivalent projection from the system + /// @returns 0 if no equivalent projection found + const Projection* _getEquiv(const Projection& proj) const; + + /// Make a clone of proj, copying across child references from the original + const Projection* _clone(const ProjectionApplier& parent, + const Projection& proj); + + /// Internal function to do the registering + const Projection* _register(const ProjectionApplier& parent, + const Projection& proj, + const string& name); + + /// Get a string dump of the current ProjHandler structure + string _getStatus() const; - /// Attach and retrieve a cloned projection as a pointer. Calls - /// {@c declareClone(oldproj, newproj)} before registering @a newproj. - const Projection* registerClonedProjection(const ProjectionApplier& parent, - const Projection* oldproj, - const Projection* newproj, - const string& name) { - if (!oldproj || !newproj) return 0; - return ®isterClonedProjection(parent, *oldproj, *newproj, name); - } + /// Check that this parent projection doesn't already use this name + bool _checkDuplicate(const ProjectionApplier& parent, + const Projection& proj, + const string& name) const; //@} - /// Enum to specify depth of projection search. - enum ProjDepth { SHALLOW, DEEP }; - + public: /// @name Projection retrieval. */ //@{ + /// Retrieve a named projection for the given parent. Returning as a /// reference is partly to discourage ProjectionApplier classes from storing /// pointer members to the registered projections, since that can lead to @@ -114,6 +167,7 @@ ProjDepth depth=SHALLOW) const; //@} + /// Projection clearing method: deletes all known projections and empties /// the reference collections. void clear(); @@ -121,12 +175,18 @@ private: + /// Remove a ProjectionApplier: designed to only be called by ~ProjectionApplier (as a friend) + void removeProjectionApplier(ProjectionApplier& parent); + + + private: + /// Get a logger. Log& getLog() const; - /// Get map of named projections belonging to @a parent. - /// Throws an exception if @a parent has not got any registered projections. + // /// Get map of named projections belonging to @a parent. + // /// Throws an exception if @a parent has not got any registered projections. // const NamedProjs& namedProjs(const ProjectionApplier* parent) const { // NamedProjsMap::const_iterator nps = _namedprojs.find(parent); // if (nps == _namedprojs.end()) { @@ -138,41 +198,6 @@ // } - private: - - /// Typedef for Projection pointer, to allow conversion to a smart pointer in this context. - typedef const Projection* ProjHandle; - - /// Typedef for a vector of Projection pointers. - typedef vector<ProjHandle> ProjHandles; - - /// @brief Typedef for the structure used to contain named projections for a - /// particular containing Analysis or Projection. - /// @todo Use a shared_pointer class? - typedef map<const string, ProjHandle> NamedProjs; - - /// Structure used to map a containing Analysis or Projection to its set of - /// contained projections. - typedef map<const ProjectionApplier*, NamedProjs> NamedProjsMap; - - /// Core data member, associating a given containing class (via a - /// ProjectionApplier pointer) to its contained projections. - NamedProjsMap _namedprojs; - - /// Cache of {@link Projection}s for reverse lookup, to speed up registering - /// new projections as @c _namedprojs gets large. - ProjHandles _projs; - - private: - - /// Private destructor means no inheritance from this class. - ~ProjectionHandler(); - - /// The assignment operator is hidden. - ProjectionHandler& operator=(const ProjectionHandler&); - - /// The copy constructor is hidden. - ProjectionHandler(const ProjectionHandler&); }; Modified: trunk/src/AnalysisLoader.cc ============================================================================== --- trunk/src/AnalysisLoader.cc Sat Nov 7 00:56:50 2009 (r2034) +++ trunk/src/AnalysisLoader.cc Sat Nov 7 01:05:38 2009 (r2035) @@ -98,7 +98,6 @@ Log::getLog("Rivet.AnalysisLoader") << Log::TRACE << "Candidate analysis plugin libs: " << pluginfiles << endl; foreach (const string& pf, pluginfiles) { Log::getLog("Rivet.AnalysisLoader") << Log::TRACE << "Trying to load plugin analyses from file " << pf << endl; - /// @todo Still needs lazy loading with new plugin design? void* handle = dlopen(pf.c_str(), RTLD_LAZY); if (!handle) { Log::getLog("Rivet.AnalysisLoader") << Log::WARN << "Cannot open " << pf << ": " << dlerror() << endl; Modified: trunk/src/ProjectionApplier.cc ============================================================================== --- trunk/src/ProjectionApplier.cc Sat Nov 7 00:56:50 2009 (r2034) +++ trunk/src/ProjectionApplier.cc Sat Nov 7 01:05:38 2009 (r2035) @@ -11,6 +11,11 @@ { } + ProjectionApplier::~ProjectionApplier() { + getProjHandler().removeProjectionApplier(*this); + } + + const Projection& ProjectionApplier::_applyProjection(const Event& evt, const string& name) const { return evt.applyProjection(getProjection(name)); @@ -23,4 +28,11 @@ } + const Projection& ProjectionApplier::_addProjection(const Projection& proj, + const std::string& name) { + const Projection& reg = getProjHandler().registerProjection(*this, proj, name); + return reg; + } + + } Modified: trunk/src/ProjectionHandler.cc ============================================================================== --- trunk/src/ProjectionHandler.cc Sat Nov 7 00:56:50 2009 (r2034) +++ trunk/src/ProjectionHandler.cc Sat Nov 7 01:05:38 2009 (r2035) @@ -12,31 +12,31 @@ ProjectionHandler* ProjectionHandler::_instance = 0; + ProjectionHandler* ProjectionHandler::create() { if (!_instance) { _instance = new ProjectionHandler(); - Log::getLog("Rivet.ProjectionHandler") + Log::getLog("Rivet.ProjectionHandler") << Log::TRACE << "Created new ProjectionHandler at " << _instance << endl; } return _instance; } + // Get a logger. Log& ProjectionHandler::getLog() const { return Log::getLog("Rivet.ProjectionHandler"); } + void ProjectionHandler::clear() { - foreach (ProjHandles::value_type& ph, _projs) { - getLog() << Log::TRACE << "Deleting projection at " << ph << endl; - delete ph; - } _projs.clear(); _namedprojs.clear(); } + // Delete contained pointers. ProjectionHandler::~ProjectionHandler() { @@ -44,119 +44,216 @@ } - void ProjectionHandler::declareClone(const Projection* oldproj, const Projection* newproj) { - if (oldproj == newproj) return; - NamedProjsMap::const_iterator nps = _namedprojs.find(oldproj); - if (nps != _namedprojs.end()) { - getLog() << Log::TRACE << "Cloning registered projections list: " - << oldproj << " -> " << newproj << endl; - _namedprojs[newproj] = nps->second; + + // Take a Projection, compare it to the others on record, and return (by + // reference) an equivalent Projection which is guaranteed to be the + // (persistent) version that will be applied to an event. + const Projection& ProjectionHandler::registerProjection(const ProjectionApplier& parent, + const Projection& proj, + const string& name) + { + getLog() << Log::TRACE << "Trying to register" + << " projection " << &proj << " (" << proj.name() << ")" + << " for parent " << &parent << " (" << parent.name() << ")" + << " with name '" << name << "'" << endl; + + // Check for duplicate use of "name" on "parent" + const bool dupOk = _checkDuplicate(parent, proj, name); + if (!dupOk) exit(1); + + // Choose which version of the projection to register with this parent and name + const Projection* p = _getEquiv(proj); + if (p == 0) { // a null pointer is a non-match + // If there is no equivalent projection, clone proj and use the clone for registering + p = _clone(parent, proj); } + + // Do the registering + p = _register(parent, *p, name); + + // Return registered proj + return *p; } - // Take a {@c clone}'d Projection, compare it to the others on record, and - // return (by reference) an equivalent Projection which is guaranteed to be - // the version that will be applied to an event. - const Projection& ProjectionHandler::registerClonedProjection(const ProjectionApplier& parent, - const Projection& oldproj, - const Projection& newproj, - const string& name) { - declareClone(&oldproj, &newproj); - return registerProjection(parent, newproj, name); + + + // Attach and retrieve a projection as a pointer. + const Projection* ProjectionHandler::registerProjection(const ProjectionApplier& parent, + const Projection* proj, + const string& name) { + if (proj == 0) return 0; + const Projection& p = registerProjection(parent, *proj, name); + return &p; } + + // Clone neatly + const Projection* ProjectionHandler::_clone(const ProjectionApplier& parent, + const Projection& proj) + { + // All is well so far, so we clone a new copy of the passed projection on the heap + getLog() << Log::TRACE << "Cloning projection " << proj.name() << " from " << &proj << endl; + const Projection* newproj = proj.clone(); + getLog() << Log::TRACE << "Cloned projection " << proj.name() << " at " << newproj << endl; + + // Copy all the child ProjHandles when cloning, since otherwise links to "stack parents" + // will be generated by their children, without any connection to the cloned parent + if (&proj != newproj) { + NamedProjsMap::const_iterator nps = _namedprojs.find(&proj); + if (nps != _namedprojs.end()) { + getLog() << Log::TRACE << "Cloning registered projections list: " + << &proj << " -> " << newproj << endl; + _namedprojs[newproj] = nps->second; + } + } + + return newproj; + } + + + // Take a Projection, compare it to the others on record, and // return (by reference) an equivalent Projection which is guaranteed to be // the version that will be applied to an event. - const Projection& ProjectionHandler::registerProjection(const ProjectionApplier& parent, - const Projection& proj, - const string& name) { - getLog() << Log::TRACE << "Trying to register" - << " projection " << &proj << "(" << proj.name() << ")" - << " for parent " << &parent << "(" << parent.name() << ")" - << " with name '" << name << "'" << endl; - - // Try to find an exact match by pointer - if (find(_projs.begin(), _projs.end(), &proj) != _projs.end()) { - getLog() << Log::TRACE << "Matched " << &proj - << " by pointer: no cmp loop needed" << endl; - _namedprojs[&parent][name] = &proj; - return proj; + const Projection* ProjectionHandler::_register(const ProjectionApplier& parent, + const Projection& proj, + const string& name) + { + // All is well so far, so we clone a new copy of the passed projection on the heap + getLog() << Log::TRACE << "Cloning projection " << proj.name() << " from " << &proj << endl; + const Projection* newproj = proj.clone(); + getLog() << Log::TRACE << "Cloned projection " << proj.name() << " at " << newproj << endl; + + // Copy all the child ProjHandles when cloning, since otherwise links to "stack parents" + // will be generated by their children, without any connection to the cloned parent + if (&proj != newproj) { + NamedProjsMap::const_iterator nps = _namedprojs.find(&proj); + if (nps != _namedprojs.end()) { + getLog() << Log::TRACE << "Cloning registered projections list: " + << &proj << " -> " << newproj << endl; + _namedprojs[newproj] = nps->second; + } } + + ProjHandle ph(newproj); + getLog() << Log::TRACE << "Registering new projection at " << ph << endl; + + // Add the passed Projection to _projs + _projs.insert(ph); + + // Add the ProjApplier* => name location to the associative container + _namedprojs[&parent][name] = ph; + + return ph.get(); + } + + + + // Try to find a equivalent projection in the system + const Projection* ProjectionHandler::_getEquiv(const Projection& proj) const + { // Get class type using RTTI const std::type_info& newtype = typeid(proj); getLog() << Log::TRACE << "RTTI type of " << &proj << " is " << newtype.name() << endl; - // Compare to ALL projections (use caching _projs). + // Compare to ALL projections via _projs collection getLog() << Log::TRACE << "Comparing " << &proj << " with " << _projs.size() << " registered projections" << endl; foreach (const ProjHandle& ph, _projs) { // Make sure the concrete types match, using RTTI. const std::type_info& regtype = typeid(*ph); - getLog() << Log::TRACE << "RTTI type comparison with "<< ph << ": " + getLog() << Log::TRACE << "RTTI type comparison with " << ph << ": " << newtype.name() << " vs. " << regtype.name() << endl; if (newtype != regtype) continue; getLog() << Log::TRACE << "RTTI type matches with " << ph << endl; - // If we find a match, ~~delete the passed object, then~~ make a copy of the - // existing pointer to the store location indexed by ProjApplier* => name. + // Test for semantic match if (pcmp(*ph, proj) != PCmp::EQUIVALENT) { - getLog() << Log::TRACE << "Type-matched projections at " + getLog() << Log::TRACE << "Projections at " << &proj << " and " << ph << " are not equivalent" << endl; } else { - getLog() << Log::TRACE << "Deleting equivalent projection at " - << &proj << " and returning " << ph << endl; - //delete &proj; - _namedprojs[&parent][name] = ph; - return *ph; + getLog() << Log::TRACE << "MATCH! Projections at " + << &proj << " and " << ph << " are equivalent" << endl; + return ph.get(); + } + } + + // If no match, just return a null pointer + return 0; + } + + + + string ProjectionHandler::_getStatus() const { + ostringstream msg; + msg << "Current projection hierarchy:" << endl; + foreach (const NamedProjsMap::value_type& nps, _namedprojs) { + //const string parentname = nps.first->name(); + msg << nps.first << endl; //"(" << parentname << ")" << endl; + foreach (const NamedProjs::value_type& np, nps.second) { + msg << " " << np.second << " (" << np.second->name() + << ", locally called '" << np.first << "')" << endl; } + msg << endl; } + return msg.str(); + } + - /// @todo Reinstate this check, since something is going wrong with undead projections! - // // If there is no match, check that the same parent hasn't already used this name for something else - // if (_namedprojs[&parent].find(name) != _namedprojs[&parent].end()) { - // getLog() << Log::ERROR << "Projection clash! " - // << parent.name() << " (" << &parent << ") " - // << "is trying to overwrite its registered '" << name << "' " - // << "projection (" << _namedprojs[&parent][name] << "=" - // << _namedprojs[&parent][name]->name() << ") with a non-equivalent projection " - // << "(" << &proj << "=" << proj.name() << ")" << endl; - // ostringstream msg; - // msg << "Current projection hierarchy:" << endl; - // foreach (const NamedProjsMap::value_type& nps, _namedprojs) { - // //const string parentname = nps.first->name(); - // msg << nps.first << endl; //"(" << parentname << ")" << endl; - // foreach (const NamedProjs::value_type& np, nps.second) { - // msg << " " << np.second << " (" << np.second->name() - // << ", locally called '" << np.first << "')" << endl; - // } - // msg << endl; - // } - // getLog() << Log::ERROR << msg.str(); - // exit(1); - // } - - - // If we found no match, and the name is free, add the passed Projection to _projs, and - // add the ProjApplier* => name location to the associative container. - getLog() << Log::TRACE << "Registered new projection at " << &proj << endl; - _projs.push_back(&proj); - _namedprojs[&parent][name] = &proj; - return proj; + // Check that the same parent hasn't already used this name for something else + bool ProjectionHandler::_checkDuplicate(const ProjectionApplier& parent, + const Projection& proj, + const string& name) const + { + NamedProjsMap::const_iterator ipnps = _namedprojs.find(&parent); + if (ipnps != _namedprojs.end()) { + const NamedProjs pnps = ipnps->second; + const NamedProjs::const_iterator ipph = pnps.find(name); + if (ipph != pnps.end()) { + const ProjHandle pph = ipph->second; + getLog() << Log::ERROR << "Projection clash! " + << parent.name() << " (" << &parent << ") " + << "is trying to overwrite its registered '" << name << "' " + << "projection (" << pph << "=" + << pph->name() << ") with a non-equivalent projection " + << "(" << &proj << "=" << proj.name() << ")" << endl; + getLog() << Log::ERROR << _getStatus(); + return false; + } + } + return true; } + + + void ProjectionHandler::removeProjectionApplier(ProjectionApplier& parent) { + NamedProjsMap::iterator npi = _namedprojs.find(&parent); + if (npi != _namedprojs.end()) _namedprojs.erase(npi); + // + const Projection* parentprojptr = dynamic_cast<Projection*>(&parent); + if (parentprojptr) { + ProjHandle parentph(parentprojptr); + ProjHandles::iterator pi = find(_projs.begin(), _projs.end(), parentph); + if (pi != _projs.end()) _projs.erase(pi); + } + } + + + + set<const Projection*> ProjectionHandler::getChildProjections(const ProjectionApplier& parent, - ProjDepth depth) const { + ProjDepth depth) const + { set<const Projection*> toplevel; NamedProjs nps = _namedprojs.find(&parent)->second; - for (NamedProjs::const_iterator np = nps.begin(); np != nps.end(); ++np) { - toplevel.insert(np->second); + foreach (NamedProjs::value_type& np, nps) { + toplevel.insert(np.second.get()); } if (depth == SHALLOW) { // Only return the projections directly contained within the top level @@ -164,10 +261,9 @@ } else { // Return recursively built projection list set<const Projection*> alllevels = toplevel; - for (set<const Projection*>::const_iterator pp = toplevel.begin(); pp != toplevel.end(); ++pp) { - set<const Projection*> allsublevels = getChildProjections(**pp, DEEP); + foreach (const Projection* p, toplevel) { + set<const Projection*> allsublevels = getChildProjections(*p, DEEP); alllevels.insert(allsublevels.begin(), allsublevels.end()); - break; } return alllevels; } @@ -175,6 +271,7 @@ + const Projection& ProjectionHandler::getProjection(const ProjectionApplier& parent, const string& name) const { getLog() << Log::TRACE << "Searching for child projection '" Modified: trunk/src/Run.cc ============================================================================== --- trunk/src/Run.cc Sat Nov 7 00:56:50 2009 (r2034) +++ trunk/src/Run.cc Sat Nov 7 01:05:38 2009 (r2035) @@ -33,7 +33,7 @@ if (evtfile == "-") { m_io = new HepMC::IO_GenEvent(std::cin); } else { - // Ignore the HepMC::IO_GenEvent(filename, ios) constructor, since only available from HepMC 2.4 + // Ignore the HepMC::IO_GenEvent(filename, ios) constructor, since it's only available from HepMC 2.4 m_istr = new std::fstream(evtfile.c_str(), std::ios::in); m_io = new HepMC::IO_GenEvent(*m_istr); } @@ -117,7 +117,7 @@ else if (evt->cross_section()) { const double xs = evt->cross_section()->cross_section(); //< in pb Log::getLog("Rivet.Run") << Log::DEBUG - << "Setting cross-section = " << xs << " pb" << endl; + << "Setting cross-section = " << xs << " pb" << endl; _ah.setCrossSection(xs); } #endif
More information about the Rivet-svn mailing list |