Skip to content

Conversation

@mateofio
Copy link
Contributor

@mateofio mateofio commented Aug 21, 2020

Depends on: EasyRPG/liblcf#384

This adds StringView in many places to make code generic to different string types.

Utils::ReplacePlaceholders has been greatly optimizing by removing all unnecessary copies from it's interface.

@mateofio mateofio added the Performance Related to improvements on memory, less CPU or disk usage, battery savings, etc. label Aug 21, 2020
@mateofio mateofio force-pushed the sv branch 2 times, most recently from e61c30e to af8c97b Compare August 21, 2020 23:52
@mateofio
Copy link
Contributor Author

mateofio commented Aug 21, 2020

Here are performance numbers for Utils::ReplacePlaceholders before and after the stringview/span conversion

with std::string and std::vector

-----------------------------------------------------------------
Benchmark                       Time             CPU   Iterations
-----------------------------------------------------------------
BM_ReplacePlaceholders        136 ns          135 ns      4977778

with StringView and Span

-----------------------------------------------------------------
Benchmark                       Time             CPU   Iterations
-----------------------------------------------------------------
BM_ReplacePlaceholders       94.1 ns         94.2 ns      7466667

So 30% of the time in this function was being spent just allocating and freeing temporary arrays.

@fdelapena fdelapena added this to the 0.6.3 milestone Aug 22, 2020
@fdelapena fdelapena added the Has PR Dependencies This PR depends on another PR label Aug 23, 2020
@mateofio
Copy link
Contributor Author

Played some rm2k games with the battle system testing the changes to wordwrap and replaceplaceholders. Everything seem fine.

Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

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

Also have to learn about span, never used them before.

* @return The async request.
*/
FileRequestAsync* RequestFile(const std::string& folder_name, const std::string& file_name);
FileRequestAsync* RequestFile(StringView folder_name, StringView file_name);
Copy link
Member

Choose a reason for hiding this comment

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

Question: So, a string view is always used when the target function does not take ownership on the string?

Copy link
Contributor Author

@mateofio mateofio Aug 24, 2020

Choose a reason for hiding this comment

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

I wish I could tell you yes, replace all const std::string& with StringView. Unfortunately this is C++ so there is always one big caveat.

StringView is not null terminated, so if at the end of your call stack you end up calling a C library which requires null terminated const char*, you'll end up having to make a copy so you can get a new string with a null byte at the end.

Aside from that caveat, yes use StringView where-ever you need a non-owning reference. Not only is it a generic adapter but the calling convention used is faster (see below).

Span does not have this caveat, fortunately so you can use it anywhere you'd pass a vector by reference.

Performance Note

const string& if you think about it in C terms, is essentially a double pointer const char**. The reference is a pointer to the string object, which contains another pointer to the bytes.

When you pass a StringView you're passing a const char*, removing one level of indirection. In some cases this can help the optimizer with it's aliasing analysis to produce faster code.

The same rule applies to vector and Span.

src/cache.cpp Outdated
std::string MakeHashKey(const std::string& folder_name, const std::string& filename, bool transparent) {
return folder_name + ":" + filename + ":" + (transparent ? "T" : " ");
std::string MakeHashKey(StringView folder_name, StringView filename, bool transparent) {
return std::string(folder_name) + ":" + std::string(filename) + ":" + (transparent ? "T" : " ");
Copy link
Member

Choose a reason for hiding this comment

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

Question: Couldn't you also use ToString() here instead of std::string(). Does it make a difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh this was from the other PR before I added ToString. Will fix.

They do the same thing in this case, but ToString is easier to grep for.

{'V', 'U'},
{std::to_string(exp), lcf::Data::terms.exp_short}
Utils::MakeArray('V', 'U'),
Utils::MakeSvArray(std::to_string(exp), lcf::Data::terms.exp_short)
Copy link
Member

Choose a reason for hiding this comment

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

Question: What's the advantage of using Make*()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I don't have to say std::array<char,2>('V', 'U')

This was actually supposed to go in the standard library, but they dropped it after adding deduction guides in C++17.

https://en.cppreference.com/w/cpp/experimental/make_array

@Ghabry Ghabry merged commit 1e5be24 into EasyRPG:master Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Performance Related to improvements on memory, less CPU or disk usage, battery savings, etc.

Development

Successfully merging this pull request may close these issues.

3 participants