Skip to content
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

Add str_from_int function #7056

Merged
merged 1 commit into from Aug 25, 2023

Conversation

Robyt3
Copy link
Member

@Robyt3 Robyt3 commented Aug 21, 2023

Add more efficient function for formatting integer values as strings.

A benchmark shows that using this function is significantly faster than using str_format. It is faster by a factor of 220 with Clang 15.0 O2 (https://quick-bench.com/q/BlNoLnlyqxipf4jvsFTUxKMHDJU) and by a factor of 11 with GCC 12.2 O2 (https://quick-bench.com/q/Fxf9lDCTqXBF4pIa_IyZ5R0IqYg).

This increases FPS in the editor by ~25% when many numbers are rendered for switch/tele/speedup/tune layers or with "Show Info" being enabled.

The additional static analysis for std::to_chars revealed that the wrong size was used in CHud for aScoreTeam[TEAM_RED] and aScoreTeam[TEAM_BLUE].

This requires incrementing the macOS deployment target from 10.13 to 10.15.

Checklist

  • Tested the change ingame
  • Provided screenshots if it is a visual change
  • Tested in combination with possibly related configuration options
  • Written a unit test (especially base/) or added coverage to integration test
  • Considered possible null pointers and out of bounds array indexing
  • Changed no physics that affect existing maps
  • Tested the change with ASan+UBSan or valgrind's memcheck (optional)

src/base/system.h Outdated Show resolved Hide resolved
Copy link
Member

@heinrich5991 heinrich5991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This increases FPS in the editor by ~25% when many numbers are rendered for switch/tele/speedup/tune layers or with "Show Info" being enabled.

That's very nice numbers. :o Thanks for pursuing this.

I wonder whether this could be made drop-in, i.e. check if the format parameter of str_format is "%d" and if so, call std::to_chars?

CMakeLists.txt Show resolved Hide resolved
src/base/system.h Show resolved Hide resolved
src/base/system.h Outdated Show resolved Hide resolved
@Robyt3
Copy link
Member Author

Robyt3 commented Aug 21, 2023

I wonder whether this could be made drop-in, i.e. check if the format parameter of str_format is "%d" and if so, call std::to_chars?

This would be 1.7x slower with GCC (https://quick-bench.com/q/zYJQrmQFwC6HUPagPduDs5ooqQ0) and 36x slower with Clang (https://quick-bench.com/q/D-aRHQtGDCCSPRNTFujS3W_2rYU).

We'd still need to use varargs, which is always slower than passing a single integer by value.

@Learath2
Copy link
Member

How about a constexpr version of format? You might be able to match the performance that way

@heinrich5991
Copy link
Member

Cool site! :o

The Clang benchmarks look moot, it inlined the function completely, i.e. it's no longer converting strings to ints in the benchmark at runtime.

We'd still need to use varargs, which is always slower than passing a single integer by value.

Even though this shouldn't™ be the case, it apparently is. I tried to look at it with godbolt and the compilers generated shitty code for variable args.

@heinrich5991
Copy link
Member

How about a constexpr version of format? You might be able to match the performance that way

This will make compile times slower though, I don't think that's desirable.

@Robyt3 Robyt3 force-pushed the Base-Number-Format-Optimization branch 2 times, most recently from 14b04da to f59c8eb Compare August 22, 2023 15:46
src/base/system.h Outdated Show resolved Hide resolved
@Robyt3 Robyt3 force-pushed the Base-Number-Format-Optimization branch from f59c8eb to bc0472a Compare August 22, 2023 17:26
@Robyt3 Robyt3 changed the title Add str_format_integer Add str_from_int function Aug 22, 2023
Copy link
Member

@heinrich5991 heinrich5991 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

@Robyt3 Robyt3 force-pushed the Base-Number-Format-Optimization branch 3 times, most recently from d2916b6 to 1b38a03 Compare August 23, 2023 15:12
@heinrich5991 heinrich5991 added this pull request to the merge queue Aug 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 23, 2023
Add more efficient function for formatting integer values as strings.

A benchmark shows that using this function is significantly faster than using `str_format`. It is faster by a factor of 220 with Clang 15.0 O2 (https://quick-bench.com/q/BlNoLnlyqxipf4jvsFTUxKMHDJU) and by a factor of 11 with GCC 12.2 O2 (https://quick-bench.com/q/Fxf9lDCTqXBF4pIa_IyZ5R0IqYg).

This increases FPS in the editor by ~25% when many numbers are rendered for switch/tele/speedup/tune layers or with "Show Info" being enabled.

The additional static analysis for `std::to_chars` revealed that the wrong size was used in `CHud` for `aScoreTeam[TEAM_RED]` and `aScoreTeam[TEAM_BLUE]`.

This requires incrementing the macOS deployment target from 10.13 to 10.15.
@Robyt3 Robyt3 force-pushed the Base-Number-Format-Optimization branch from 1b38a03 to d2c9750 Compare August 24, 2023 18:54
@heinrich5991 heinrich5991 added this pull request to the merge queue Aug 25, 2023
Merged via the queue into ddnet:master with commit b739c18 Aug 25, 2023
14 checks passed
@Robyt3 Robyt3 deleted the Base-Number-Format-Optimization branch August 25, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants