diff --git a/src/generic_factory.h b/src/generic_factory.h index 53fafaac15210..3c58b701347ec 100644 --- a/src/generic_factory.h +++ b/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 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,13 +336,12 @@ 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; } @@ -344,8 +349,7 @@ class generic_factory 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( i ); - list[i].id._version = version; + list[i].id.set_cid_version( static_cast( 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. diff --git a/src/string_id.h b/src/string_id.h index 21a7f9927c748..4d95687d23d1c 100644 --- a/src/string_id.h +++ b/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; }; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index bfd56f6ecaf3a..5255282788e62 100644 --- a/tests/CMakeLists.txt +++ b/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) diff --git a/tests/Makefile b/tests/Makefile index 123e57e943896..c12466685bd98 100644 --- a/tests/Makefile +++ b/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 diff --git a/tests/generic_factory_test.cpp b/tests/generic_factory_test.cpp index 29885e9cf6c5e..7b64c1288401f 100644 --- a/tests/generic_factory_test.cpp +++ b/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() ); -} \ No newline at end of file +} + +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_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( -1 ) ); + } + if( second_cached ) { + test_factory.convert( id2, int_id( -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_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_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