Follow up to #44261: string_id performance and correctness

msvc-full-features/Cataclysm-test-vcpkg-static.vcxproj

@@ -81,7 +81,7 @@
       <DisableSpecificWarnings>4819;4146;26495;26444;26451;4068;6319;6237</DisableSpecificWarnings>
       <ForcedIncludeFiles>stdafx.h</ForcedIncludeFiles>
       <AdditionalOptions>/bigobj /utf-8 %(AdditionalOptions)</AdditionalOptions>
-      <PreprocessorDefinitions>_SCL_SECURE_NO_WARNINGS;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_CONSOLE;SDL_SOUND;TILES;SDL_MAIN_HANDLED;LOCALIZE;USE_VCPKG;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>_SCL_SECURE_NO_WARNINGS;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_CONSOLE;SDL_SOUND;TILES;SDL_MAIN_HANDLED;LOCALIZE;USE_VCPKG;CATCH_CONFIG_ENABLE_BENCHMARKING;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <LanguageStandard>stdcpp14</LanguageStandard>
       <AdditionalIncludeDirectories>..\src;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ClCompile>

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;
+        int64_t  version = 0;
+
+        void inc_version() {
+            do {
+                version++;
+            } while( version == INVALID_VERSION );
+        }
 
     protected:
         std::vector<T> list;
@@ -145,14 +151,13 @@ 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( INVALID_CID, version );
                 return false;
             }
             result = iter->second;
-            id._cid = result.to_i();
+            id.set_cid_version( result.to_i(), version );
             return true;
         }
 
@@ -330,22 +335,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 +358,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 +391,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

@@ -5,6 +5,9 @@
 #include <string>
 #include <type_traits>
 
+static constexpr int64_t INVALID_VERSION = -1;
+static constexpr int INVALID_CID = -1;
+
 template<typename T>
 class int_id;
 
@@ -77,13 +80,14 @@ class string_id
         // a std::string, otherwise a "no matching function to call..." error is generated.
         template<typename S, class = typename
                  std::enable_if< std::is_convertible<S, std::string >::value>::type >
-        explicit string_id( S && id ) : _id( std::forward<S>( id ) ), _cid( -1 ), _version( -1 ) {}
+        explicit string_id( S &&
+                            id ) : _id( std::forward<S>( id ) ), _cid( INVALID_CID ), _version( INVALID_VERSION ) {}
         /**
          * Default constructor constructs an empty id string.
          * Note that this id class does not enforce empty id strings (or any specific string at all)
          * to be special. Every string (including the empty one) may be a valid id.
          */
-        string_id() : _cid( -1 ), _version( -1 ) {}
+        string_id() : _cid( INVALID_CID ), _version( INVALID_VERSION ) {}
         /**
          * Comparison, only useful when the id is used in std::map or std::set as key. Compares
          * the string id as with the strings comparison.
@@ -95,13 +99,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 != INVALID_CID || rhs._cid != INVALID_CID ) &&
+                                   _version == rhs._version && _version != INVALID_VERSION;
+            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 +207,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 int64_t _version;
+
+        inline void set_cid_version( int cid, int64_t 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)
 
+	# Enabling 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.
 
+# Enabling 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,114 @@ 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 ) {
+        // is_valid should update cache internally
+        test_factory.is_valid( id1 );
+    }
+    if( second_cached ) {
+        test_factory.is_valid( id2 );
+    }
+
+    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 );
+        }
+    }
+}
+
+// Benchmarks are skipped by default by using [.] tag
+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.is_valid( id_200 );
+    test_factory.is_valid( id_200_ );
+    test_factory.is_valid( 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;
+    };
+}