-
Notifications
You must be signed in to change notification settings - Fork 189
perf(ini): Improve INI parsing performance by using std::from_chars #2532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,16 @@ | |
| #include "GameLogic/ScriptEngine.h" | ||
| #include "GameLogic/Weapon.h" | ||
|
|
||
| #if (defined(_MSC_VER) && _MSC_VER < 1300) | ||
| #define USE_STD_FROM_CHARS_PARSING 0 | ||
| #else | ||
| #define USE_STD_FROM_CHARS_PARSING 1 | ||
| #endif | ||
|
|
||
| #if USE_STD_FROM_CHARS_PARSING | ||
| #include <charconv> | ||
| #include <type_traits> | ||
| #endif | ||
|
|
||
| /////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| // PRIVATE DATA /////////////////////////////////////////////////////////////////////////////////// | ||
|
|
@@ -1625,40 +1635,87 @@ void INI::initFromINIMulti( void *what, const MultiIniFieldParse& parseTableList | |
| return TheScienceStore->friend_lookupScience( token ); | ||
| } | ||
|
|
||
| #if USE_STD_FROM_CHARS_PARSING | ||
|
|
||
| template <typename Type> | ||
| Type scanType(const char* token) | ||
|
Caball009 marked this conversation as resolved.
Outdated
|
||
| { | ||
| // TheSuperHackers @info std::from_chars cannot parse "-1" as uint32 so the result needs to be a 64 bit type for integers. | ||
| std::conditional_t<std::is_integral_v<Type>, Int64, Real> result{}; | ||
| // TheSuperHackers @todo Try to optimize this strlen call away by using std::string_view or similar. | ||
| const auto [ptr, ec] = std::from_chars(token, token + strlen(token), result); | ||
|
|
||
| if (ec != std::errc{}) | ||
| { | ||
| throw INI_INVALID_DATA; | ||
| } | ||
|
|
||
| return static_cast<Type>(result); | ||
| } | ||
|
|
||
| #endif | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| /*static*/ Int INI::scanInt(const char* token) | ||
| { | ||
| #if USE_STD_FROM_CHARS_PARSING == 1 | ||
| return scanType<Int>(token); | ||
| #else | ||
| Int value; | ||
| if (sscanf( token, "%d", &value ) != 1) | ||
| throw INI_INVALID_DATA; | ||
|
|
||
| #if USE_STD_FROM_CHARS_PARSING == -1 | ||
| if (value != scanType<Int>(token)) | ||
| throw INI_INVALID_DATA; | ||
| #endif | ||
|
|
||
| return value; | ||
| #endif | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| /*static*/ UnsignedInt INI::scanUnsignedInt(const char* token) | ||
| { | ||
| #if USE_STD_FROM_CHARS_PARSING == 1 | ||
| return scanType<UnsignedInt>(token); | ||
| #else | ||
| UnsignedInt value; | ||
| if (sscanf( token, "%u", &value ) != 1) // unsigned int is %u, not %d | ||
| throw INI_INVALID_DATA; | ||
|
|
||
| #if USE_STD_FROM_CHARS_PARSING == -1 | ||
| if (value != scanType<UnsignedInt>(token)) | ||
| throw INI_INVALID_DATA; | ||
| #endif | ||
|
|
||
| return value; | ||
| #endif | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| /*static*/ Real INI::scanReal(const char* token) | ||
| { | ||
| #if USE_STD_FROM_CHARS_PARSING == 1 | ||
| return scanType<Real>(token); | ||
| #else | ||
| Real value; | ||
| if (sscanf( token, "%f", &value ) != 1) | ||
| throw INI_INVALID_DATA; | ||
|
|
||
| #if USE_STD_FROM_CHARS_PARSING == -1 | ||
| if (value != scanType<Real>(token)) | ||
| throw INI_INVALID_DATA; | ||
|
Comment on lines
+1706
to
+1708
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider using an epsilon comparison or checking that the two results round-trip to the same string rather than requiring bitwise identity. Prompt To Fix With AIThis is a comment left during a code review.
Path: Core/GameEngine/Source/Common/INI/INI.cpp
Line: 1706-1708
Comment:
**Float `!=` comparison in validation mode may produce false positives**
`sscanf("%f")` and `std::from_chars` are two independently implemented parsers. For most INI values this is fine, but for decimal strings near a floating-point midpoint (halfway rounding boundary) the two parsers can legitimately round in opposite directions and produce results that differ by 1 ULP — causing the validation mode to throw `INI_INVALID_DATA` even though both parsed the same string. If this mode is used to validate a full game initialization, those spurious exceptions will hide real data rather than proving parity.
Consider using an epsilon comparison or checking that the two results round-trip to the same string rather than requiring bitwise identity.
How can I resolve this? If you propose a fix, please make it concise. |
||
| #endif | ||
|
|
||
| return value; | ||
| #endif | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| /*static*/ Real INI::scanPercentToReal(const char* token) | ||
| { | ||
| Real value; | ||
| if (sscanf( token, "%f", &value ) != 1) | ||
| throw INI_INVALID_DATA; | ||
| return value / 100.0f; | ||
| return scanReal(token) / 100.0f; | ||
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.