diff --git a/msvc-full-features/Cataclysm-test-vcpkg-static.vcxproj b/msvc-full-features/Cataclysm-test-vcpkg-static.vcxproj index 35a5929c11e3d..c559abe964ed5 100644 --- a/msvc-full-features/Cataclysm-test-vcpkg-static.vcxproj +++ b/msvc-full-features/Cataclysm-test-vcpkg-static.vcxproj @@ -81,7 +81,7 @@ 4819;4146;26495;26444;26451;4068;6319;6237 stdafx.h /bigobj /utf-8 %(AdditionalOptions) - _SCL_SECURE_NO_WARNINGS;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_CONSOLE;SDL_SOUND;TILES;SDL_MAIN_HANDLED;LOCALIZE;USE_VCPKG;%(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) stdcpp14 ..\src;%(AdditionalIncludeDirectories) diff --git a/src/generic_factory.h b/src/generic_factory.h index 53fafaac15210..256203ee3fd42 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; + int64_t version = 0; + + void inc_version() { + do { + version++; + } while( version == INVALID_VERSION ); + } protected: std::vector 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,13 +335,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 +348,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 +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( i ); - list[i].id._version = version; + list[i].id.set_cid_version( static_cast( 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. diff --git a/src/string_id.h b/src/string_id.h index 21a7f9927c748..acabb0a8bbb97 100644 --- a/src/string_id.h +++ b/src/string_id.h @@ -5,6 +5,9 @@ #include #include +static constexpr int64_t INVALID_VERSION = -1; +static constexpr int INVALID_CID = -1; + template class int_id; @@ -77,13 +80,14 @@ class string_id // a std::string, otherwise a "no matching function to call..." error is generated. template::value>::type > - explicit string_id( S && id ) : _id( std::forward( id ) ), _cid( -1 ), _version( -1 ) {} + explicit string_id( S && + id ) : _id( std::forward( 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; }; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index bfd56f6ecaf3a..7c7053b29ad49 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) + # 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) diff --git a/tests/Makefile b/tests/Makefile index 123e57e943896..592bbb30ede42 100644 --- a/tests/Makefile +++ b/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 diff --git a/tests/generic_factory_test.cpp b/tests/generic_factory_test.cpp index 29885e9cf6c5e..7df5430b8b165 100644 --- a/tests/generic_factory_test.cpp +++ b/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() ); -} \ 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 ) { + // 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_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.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; + }; +}