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

Optimize String construction from statically known strings by evaluating strlen at compile-time. #100132

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

Ivorforce
Copy link
Member

@Ivorforce Ivorforce commented Dec 7, 2024

In this PR, String constructors are now available which make use of known string lengths, avoiding a strlen call. For example, when using string literals like String s = "Test" it is knowable at compile-time that "Test" is a 4-length string. This is used to speed up String construction.

This PR builds on #99816. There, I have tested String construction from a long shader string literal, taken from the codebase. I ran the same benchmark to see how much known-length construction speeds up allocation:

  • 335us on master (aa8d9b8)
  • 139us on #99816 (e1c4239)
  • 100us on this branch (10aac990e158592e2ed50b9dbeb513c7505128e2)

The speed-up is likely to be slightly better for short strings.

Copy link
Member

@hpvb hpvb left a comment

Choose a reason for hiding this comment

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

LGTM

@Ivorforce
Copy link
Member Author

Ivorforce commented Dec 7, 2024

I am deleting the StrRange constructors for now. They were not used yet in the PR anyway, and thus aren't relevant to it. Instead, I will make a follow-up PR to add factory methods with the same semantics instead of constructors, so it's clear how exactly the bits are treated.

@Ivorforce Ivorforce force-pushed the string-compile-time-strlen branch from 97e8c25 to d3055f8 Compare December 7, 2024 13:18
Copy link
Member

@hpvb hpvb left a comment

Choose a reason for hiding this comment

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

Still approved.

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Dec 7, 2024
@Ivorforce Ivorforce force-pushed the string-compile-time-strlen branch from d3055f8 to 0d390f0 Compare December 7, 2024 22:18
template <typename Element>
struct StrRange {
const Element *c_str;
size_t len;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have used a more explicit uint64 type. No need to use uint32 as it will be padded anyway. So it will be clearer that every size is supported.

Copy link
Member Author

@Ivorforce Ivorforce Dec 8, 2024

Choose a reason for hiding this comment

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

You mean use uint64 instead of size_t? That would be fairly unusual when dealing with arrays.

Copy link
Contributor

@kiroxas kiroxas Dec 8, 2024

Choose a reason for hiding this comment

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

Again, stylistic. But it removes ambiguity.
Unusual where ? CowData use uint64_t for its size, LocalVector uses uint32_t

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. I'm not sure why that is. Possibly because they're designed to be easily exposeable to GDScript?
Still, I think size_t is cleaner; it's very common to use that for sizes across c++.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that it is the default in C++ standard library. It's just, why be ambiguous on the size of a type when you don't have to. Size_t probably always is 64bits nowadays anyway, this is why this is a style choice. Any choice will do, just raising a point :).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll keep it as size_t for now. But let's keep the discussion open for other people to chime in!

Copy link
Contributor

Choose a reason for hiding this comment

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

Style changes in core are inherently a very low priority, so I'm not too surprised there's mixing and matching of different names/formats. A lot of the discrepancies could come down to how several templates originally used int/32-bit storage logic instead of 64-bit, but I can't say for certain. In any case, size_t is perfectly acceptable, and is arguably the best option for logic/metrics having to deal with… Well, size!

@Ivorforce Ivorforce force-pushed the string-compile-time-strlen branch 3 times, most recently from 176b6d5 to 84c71d0 Compare December 8, 2024 10:40
@Ivorforce
Copy link
Member Author

Sorry for repeated pushes, I accidentally pushed an old version and had to reset -.-

…strlen` to be evaluated at compile time, where possible.
@Ivorforce Ivorforce force-pushed the string-compile-time-strlen branch from 84c71d0 to a3f48f7 Compare December 9, 2024 20:47
@Repiteo Repiteo merged commit 78215f3 into godotengine:master Dec 10, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 10, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants