Redo FactorCollection including better locking

git-svn-id: https://mosesdecoder.svn.sourceforge.net/svnroot/mosesdecoder/trunk@4242 1f5c12ca-751b-0410-a591-d2e778427230

moses/src/Factor.cpp

@@ -29,19 +29,6 @@ using namespace std;
 
 namespace Moses
 {
-Factor::Factor(FactorDirection /* direction */, FactorType /* factorType */, const std::string *factorString, size_t id)
-  ://m_direction(direction)
-//,m_factorType(factorType)
-  m_ptrString(factorString)
-  ,m_id(id)
-{}
-
-Factor::Factor(FactorDirection /* direction */, FactorType /* factorType */, const std::string *factorString)
-//:m_direction(direction)
-//,m_factorType(factorType)
-  :m_ptrString(factorString)
-  ,m_id(NOT_FOUND)
-{}
 
 TO_STRING_BODY(Factor)
 

moses/src/Factor.h

@@ -53,27 +53,14 @@ class Factor
 
 protected:
 
-  //FactorDirection		m_direction;
-  //FactorType				m_factorType;
+  // FactorCollection writes here.  
   const std::string	*m_ptrString;
-  const size_t			m_id;
+  size_t			m_id;
 
   //! protected constructor. only friend class, FactorCollection, is allowed to create Factor objects
-  Factor(FactorDirection direction, FactorType factorType, const std::string *factorString, size_t id);
-  //! no id set. do not used to create new factors, only used for seeing if factor exists
-  Factor(FactorDirection direction, FactorType factorType, const std::string *factorString);
+  Factor() {}
 
 public:
-  //! returns whether this factor is part of the source ('Input') or target ('Output') language
-  //inline FactorDirection GetFactorDirection() const
-  //{
-  //	return m_direction;
-  //}
-  //! index, FactorType. For example, 0=surface, 1=POS. The actual mapping is user defined
-  //inline FactorType GetFactorType() const
-  //{
-  //	return m_factorType;
-  //}
   //! original string representation of the factor
   inline const std::string &GetString() const {
     return *m_ptrString;
@@ -83,44 +70,21 @@ public:
     return m_id;
   }
 
-  /*
-  //! Alternative comparison between factors. Not yet used
-  inline unsigned int GetHash() const
-  {
-  	unsigned int h=quick_hash((const char*)&m_direction, sizeof(FactorDirection), 0xc7e7f2fd);
-  	h=quick_hash((const char*)&m_factorType, sizeof(FactorType), h);
-  	h=quick_hash((const char*)&m_ptrString, sizeof(const std::string *), h);
-  	return h;
-  }
-  */
-
   /** transitive comparison between 2 factors.
   *	-1 = less than
   *	+1 = more than
   *	0	= same
-  *	Used by operator< & operator==, as well as other classes
   */
   inline int Compare(const Factor &compare) const {
     if (m_ptrString < compare.m_ptrString)
       return -1;
     if (m_ptrString > compare.m_ptrString)
       return 1;
-    /*
-    		if (m_direction < compare.m_direction)
-    			return -1;
-    		if (m_direction > compare.m_direction)
-    			return 1;
-
-    		if (m_factorType < compare.m_factorType)
-    			return -1;
-    		if (m_factorType > compare.m_factorType)
-    			return 1;
-    */
     return 0;
   }
   //! transitive comparison used for adding objects into FactorCollection
   inline bool operator<(const Factor &compare) const {
-    return Compare(compare) < 0;
+    return m_ptrString < compare.m_ptrString;
   }
 
   // quick equality comparison. Not used

moses/src/FactorCollection.cpp

@@ -33,19 +33,12 @@ namespace Moses
 {
 FactorCollection FactorCollection::s_instance;
 
-bool FactorCollection::Exists(FactorDirection direction, FactorType factorType, const string &factorString)
+bool FactorCollection::Exists(FactorDirection direction, FactorType factorType, const string &factorString) const
 {
 #ifdef WITH_THREADS
   boost::shared_lock<boost::shared_mutex> lock(m_accessLock);
 #endif
-  // find string id
-  const string *ptrString=&(*m_factorStringCollection.insert(factorString).first);
-
-  FactorSet::const_iterator iterFactor;
-  Factor search(direction, factorType, ptrString); // id not used for searching
-
-  iterFactor = m_collection.find(search);
-  return iterFactor != m_collection.end();
+  return m_map.find(factorString) != m_map.end();
 }
 
 const Factor *FactorCollection::AddFactor(FactorDirection direction
@@ -53,17 +46,21 @@ const Factor *FactorCollection::AddFactor(FactorDirection direction
     , const string 		&factorString)
 {
 #ifdef WITH_THREADS
-  boost::upgrade_lock<boost::shared_mutex> lock(m_accessLock);
-  boost::upgrade_to_unique_lock<boost::shared_mutex> uniqueLock(lock);
+  {
+    boost::shared_lock<boost::shared_mutex> read_lock(m_accessLock);
+    Map::const_iterator i = m_map.find(factorString);
+    if (i != m_map.end()) return &i->second;
+  }
+  boost::unique_lock<boost::shared_mutex> lock(m_accessLock);
 #endif
-  // find string id
-  const string *ptrString=&(*m_factorStringCollection.insert(factorString).first);
-  pair<FactorSet::iterator, bool> ret = m_collection.insert( Factor(direction, factorType, ptrString, m_factorId) );
-  if (ret.second)
-    ++m_factorId; // new factor, make sure next new factor has diffrernt id
-
-  const Factor *factor = &(*ret.first);
-  return factor;
+  std::pair<std::string, Factor> to_ins(factorString, Factor());
+  std::pair<Map::iterator, bool> ret(m_map.insert(to_ins));
+  if (ret.second) {
+    Factor &factor = ret.first->second;
+    factor.m_id = m_factorId++;
+    factor.m_ptrString = &ret.first->first;
+  }
+  return &ret.first->second;
 }
 
 FactorCollection::~FactorCollection()
@@ -80,13 +77,12 @@ TO_STRING_BODY(FactorCollection);
 // friend
 ostream& operator<<(ostream& out, const FactorCollection& factorCollection)
 {
-  FactorSet::const_iterator iterFactor;
-
-  for (iterFactor = factorCollection.m_collection.begin() ; iterFactor != factorCollection.m_collection.end() ; ++iterFactor) {
-    const Factor &factor 	= *iterFactor;
-    out << factor;
+#ifdef WITH_THREADS
+  boost::shared_lock<boost::shared_mutex> lock(factorCollection.m_accessLock);
+#endif
+  for (FactorCollection::Map::const_iterator i = factorCollection.m_map.begin(); i != factorCollection.m_map.end(); ++i) {
+    out << i->second;
   }
-
   return out;
 }
 

moses/src/FactorCollection.h

@@ -22,23 +22,25 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
 #ifndef moses_FactorCollection_h
 #define moses_FactorCollection_h
 
-#include <set>
-#include <string>
+#include "config.h"
 
 #ifdef WITH_THREADS
 #include <boost/thread/shared_mutex.hpp>
 #endif
 
+#ifdef HAVE_BOOST
+#include <boost/unordered_map.hpp>
+#else
+#include <map>
+#endif
+
+#include <string>
+
 #include "Factor.h"
 
 namespace Moses
 {
 
-class LanguageModel;
-
-typedef std::set<Factor> FactorSet;
-typedef std::set<std::string> StringSet;
-
 /** collection of factors
  *
  * All Factors in moses are accessed and created by a FactorCollection.
@@ -51,16 +53,20 @@ class FactorCollection
 {
   friend std::ostream& operator<<(std::ostream&, const FactorCollection&);
 
-protected:
+#ifdef HAVE_BOOST
+  typedef boost::unordered_map<std::string, Factor> Map;
+#else
+  typedef std::map<std::string, Factor> Map;
+#endif
+  Map m_map;
+
   static FactorCollection s_instance;
 #ifdef WITH_THREADS
   //reader-writer lock
-  boost::shared_mutex m_accessLock;
+  mutable boost::shared_mutex m_accessLock;
 #endif
 
-  size_t		m_factorId; /**< unique, contiguous ids, starting from 0, for each factor */
-  FactorSet m_collection; /**< collection of all factors */
-  StringSet m_factorStringCollection; /**< collection of unique string used by factors */
+  size_t m_factorId; /**< unique, contiguous ids, starting from 0, for each factor */
 
   //! constructor. only the 1 static variable can be created
   FactorCollection()
@@ -72,11 +78,10 @@ public:
     return s_instance;
   }
 
-  //! Destructor
   ~FactorCollection();
 
   //! Test to see whether a factor exists
-  bool Exists(FactorDirection direction, FactorType factorType, const std::string &factorString);
+  bool Exists(FactorDirection direction, FactorType factorType, const std::string &factorString) const;
   /** returns a factor with the same direction, factorType and factorString.
   *	If a factor already exist in the collection, return the existing factor, if not create a new 1
   */