Skip to content

Make JsonEncode() a bit faster #10400

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Make JsonEncode() a bit faster #10400

wants to merge 2 commits into from

Conversation

Al2Klimov
Copy link
Member

@julianbrost As discussed offline, I had a look at JsonEncode() which appeared while profiling

ref/NC/820479

to make it faster.

so that e.g. `String("foo")+"bar"` re-uses `String("foo")` instead of copying.
@Al2Klimov Al2Klimov added core/quality Improve code, libraries, algorithms, inline docs ref/NC labels Apr 1, 2025
@cla-bot cla-bot bot added the cla/signed label Apr 1, 2025
@lippserd
Copy link
Member

lippserd commented Apr 1, 2025

Please add proof.

@oxzi
Copy link
Member

oxzi commented Apr 2, 2025

Could you please give this PR an actual useful title that does not bow to the alt-right?

@Al2Klimov Al2Klimov changed the title Make JSON fast again🧢 Make JsonEncode() faster Apr 2, 2025
@Al2Klimov Al2Klimov changed the title Make JsonEncode() faster Make JsonEncode() a bit faster Apr 2, 2025
Comment on lines +336 to +349
String icinga::operator+(String&& lhs, const char *rhs)
{
return std::move(lhs.GetData()) + rhs;
}

String icinga::operator+(const char *lhs, const String& rhs)
{
return lhs + rhs.GetData();
}

String icinga::operator+(const char *lhs, String&& rhs)
{
return lhs + std::move(rhs.GetData());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you get here? Like how did you get to the conclusion that this will be a useful optimization?

(Also, I really wonder if there's actually an implementation of std::string that allows prepending without reallocation (unless you prepend the empty string), but well, a corresponding overload of operator+ is provided for std::string.)

Copy link
Member Author

Choose a reason for hiding this comment

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

How did you get here? Like how did you get to the conclusion that this will be a useful optimization?

Checkout 27e1850 and Cmd+LClick the + sign in

return stateMachine.GetResult() + "\n";

CLion will open String icinga::operator+(const String&,const char*) which copies the string.

(Also, I really wonder if there's actually an implementation of std::string that allows prepending without reallocation

As long as there's enough free capacity, there's no reason to reallocate.

a corresponding overload of operator+ is provided for std::string.)

Exactly.

@Al2Klimov Al2Klimov force-pushed the make-json-fast-again branch from ebf042a to 0e647d0 Compare April 2, 2025 13:18
@Al2Klimov Al2Klimov added the good first issue Good for newcomers label Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed core/quality Improve code, libraries, algorithms, inline docs good first issue Good for newcomers ref/NC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants