Follow up to #44261: string_id performance and correctness

src/generic_factory.h

@@ -125,7 +125,13 @@ class generic_factory
         // it's incremented when any changes to the inner id containers occur
         // version value corresponds to the string_id::_version,
         // so incrementing the version here effectively invalidates all cached string_id::_cid
-        int version = 0;
+        signed long long  version = 0;
+
+        void inc_version() {
+            do {
+                version++;
+            } while( version == -1 );
+        }
 
     protected:
         std::vector<T> list;
@@ -145,14 +151,14 @@ class generic_factory
             }
             const auto iter = map.find( id );
             // map lookup happens at most once per string_id instance per generic_factory::version
-            id._version = version;
             // id was not found, explicitly marking it as "invalid"
             if( iter == map.end() ) {
                 id._cid = -1;
+                id.set_cid_version( -1, version );
                 return false;
             }
             result = iter->second;
-            id._cid = result.to_i();
+            id.set_cid_version( result.to_i(), version );
             return true;
         }
 
@@ -330,22 +336,20 @@ class generic_factory
             // (lookups for not-yet-inserted elements)
             // in the common scenario there is no loss of performance, as `finalize` will make cache
             // for all ids valid again
-            version++;
+            inc_version();
             const auto iter = map.find( obj.id );
             if( iter != map.end() ) {
                 T &result = list[iter->second.to_i()];
                 result = obj;
-                result.id._cid = iter->second.to_i();
-                result.id._version = version;
+                result.id.set_cid_version( iter->second.to_i(), version );
                 return result;
             }
 
             const int_id<T> cid( list.size() );
             list.push_back( obj );
 
             T &result = list.back();
-            result.id._cid = cid.to_i();
-            result.id._version = version;
+            result.id.set_cid_version( cid.to_i(), version );
             map[result.id] = cid;
             return result;
         }
@@ -355,10 +359,9 @@ class generic_factory
             DynamicDataLoader::get_instance().load_deferred( deferred );
             abstracts.clear();
 
-            version++;
+            inc_version();
             for( size_t i = 0; i < list.size(); i++ ) {
-                list[i].id._cid = static_cast<int>( i );
-                list[i].id._version = version;
+                list[i].id.set_cid_version( static_cast<int>( i ), version );
             }
         }
 
@@ -389,7 +392,7 @@ class generic_factory
         void reset() {
             list.clear();
             map.clear();
-            version++;
+            inc_version();
         }
         /**
          * Returns all the loaded objects. It can be used to iterate over them.

src/string_id.h

@@ -95,13 +95,19 @@ class string_id
          * The usual comparator, compares the string id as usual.
          */
         bool operator==( const This &rhs ) const {
-            return _id == rhs._id;
+            bool can_compare_cid = ( _cid != -1 || rhs._cid != -1 ) &&
+                                   _version == rhs._version && _version != -1;
+            return ( can_compare_cid && _cid == rhs._cid ) ||
+                   ( !can_compare_cid && _id == rhs._id );
+            // else returns false, when:
+            //      can_compare_cid && _cid != rhs._cid
+            //  OR  !can_compare_cid && _id != rhs._id
         }
         /**
          * The usual comparator, compares the string id as usual.
          */
         bool operator!=( const This &rhs ) const {
-            return _id != rhs._id;
+            return ! operator==( rhs );
         }
         /**
          * Interface to the plain C-string of the id. This function mimics the std::string
@@ -197,7 +203,12 @@ class string_id
         // cached int_id counterpart of this string_id
         mutable int _cid;
         // generic_factory version that corresponds to the _cid
-        mutable int _version;
+        mutable signed long long int _version;
+
+        inline void set_cid_version( int cid, signed long long int version ) const {
+            _cid = cid;
+            _version = version;
+        }
 
         friend class generic_factory<T>;
 };

tests/CMakeLists.txt

@@ -2,6 +2,9 @@ IF(BUILD_TESTING)
 	FILE(GLOB CATACLYSM_DDA_TEST_SOURCES
 		${CMAKE_SOURCE_DIR}/tests/*.cpp)
 
+	# Uncomment to compile benchmarks
+ 	# ADD_DEFINITIONS(-DCATCH_CONFIG_ENABLE_BENCHMARKING)
+
 	IF(TILES)
 		add_executable(cata_test-tiles ${CATACLYSM_DDA_TEST_SOURCES})
 		target_link_libraries(cata_test-tiles cataclysm-tiles-common)

tests/Makefile

@@ -15,6 +15,9 @@ ODIR ?= obj
 
 LDFLAGS += -L.
 
+# Uncomment to compile benchmarks
+# DEFINES = -DCATCH_CONFIG_ENABLE_BENCHMARKING
+
 # Allow use of any header files from cataclysm.
 # Catch sections throw unused variable warnings.
 # Add no-sign-compare to fix MXE issue when compiling

tests/generic_factory_test.cpp

@@ -107,4 +107,116 @@ TEST_CASE( "generic_factory_common_null_ids", "[generic_factory]" )
 
     CHECK( field_type_str_id::NULL_ID().is_null() );
     CHECK( field_type_str_id::NULL_ID().is_valid() );
-}
+}
+
+TEST_CASE( "string_ids_comparison", "[generic_factory][string_id]" )
+{
+    //  checks equality correctness for the following combinations of parameters:
+    bool equal = GENERATE( true, false ); // whether ids are equal
+    bool first_cached = GENERATE( true, false ); // whether _version is current (first id)
+    bool first_valid = GENERATE( true, false ); // whether id is present in the factory (first id)
+    bool second_cached = GENERATE( true, false ); // --- second id
+    bool second_valid = GENERATE( true, false ); // --- second id
+
+    test_obj_id id1( "id1" );
+    test_obj_id id2( ( equal ?  "id1" : "id2" ) );
+
+    generic_factory<test_obj> test_factory( "test_factory" );
+
+    // both ids must be inserted first and then cached for the test to be valid
+    // as `insert` invalidates the cache
+    if( first_valid ) {
+        test_factory.insert( { id1, "value" } );
+    }
+    if( second_valid ) {
+        test_factory.insert( { id2, "value" } );
+    }
+    if( first_cached ) {
+        test_factory.convert( id1, int_id<test_obj>( -1 ) );
+    }
+    if( second_cached ) {
+        test_factory.convert( id2, int_id<test_obj>( -1 ) );
+    }
+
+    const auto id_info = []( bool cached, bool valid ) {
+        std::ostringstream ret;
+        ret << "cached_" << cached << "__" "valid_" << valid;
+        return ret.str();
+    };
+
+    DYNAMIC_SECTION( ( equal ? "equal" : "not_equal" ) << "___" <<
+                     "first_" << id_info( first_cached, first_valid ) << "___"
+                     "second_" << id_info( second_cached, second_valid )
+                   ) {
+        if( equal ) {
+            CHECK( id1 == id2 );
+        } else {
+            CHECK_FALSE( id1 == id2 );
+        }
+    }
+}
+
+#ifdef CATCH_CONFIG_ENABLE_BENCHMARKING
+
+TEST_CASE( "generic_factory_lookup_benchmark", "[generic_factory][benchmark]" )
+{
+    test_obj_id id_200( "id_200" );
+
+    generic_factory<test_obj> test_factory( "test_factory" );
+
+    for( int i = 0; i < 1000; ++i ) {
+        std::string id = "id_" + std::to_string( i );
+        std::string value = "value_" + std::to_string( i );
+        test_factory.insert( {test_obj_id( id ), value} );
+    }
+
+    BENCHMARK( "single lookup" ) {
+        return test_factory.obj( id_200 ).value;
+    };
+}
+
+TEST_CASE( "string_id_compare_benchmark", "[generic_factory][string_id][benchmark]" )
+{
+    std::string prefix;
+    SECTION( "short id" ) {
+    }
+    SECTION( "long id" ) {
+        prefix = "log_common_prefix_";
+    }
+
+    test_obj_id id_200( prefix + "id_200" );
+    test_obj_id id_200_( prefix + "id_200" );
+    test_obj_id id_300( prefix + "id_300" );
+
+    generic_factory<test_obj> test_factory( "test_factory" );
+
+    CHECK( id_200 == id_200_ );
+    BENCHMARK( "ids are equal, invalid version" ) {
+        return id_200 == id_200_;
+    };
+
+    CHECK_FALSE( id_200 == id_300 );
+    BENCHMARK( "ids are not equal, invalid version" ) {
+        return id_200 == id_300;
+    };
+
+    test_factory.insert( {test_obj_id( id_200 ), "value_200"} );
+    test_factory.insert( {test_obj_id( id_200_ ), "value_200_"} );
+    test_factory.insert( {test_obj_id( id_300 ), "value_300"} );
+    // make _version inside the ids valid
+    test_factory.obj( id_200 );
+    test_factory.obj( id_200_ );
+    test_factory.obj( id_300 );
+
+    CHECK( id_200 == id_200_ );
+    BENCHMARK( "ids are equal, valid version" ) {
+        return id_200 == id_200_;
+    };
+
+    CHECK_FALSE( id_200 == id_300 );
+    BENCHMARK( "ids are not equal, valid version" ) {
+        return id_200 == id_300;
+    };
+}
+
+#endif // CATCH_CONFIG_ENABLE_BENCHMARKING