Skip to content

Fix string move constructor and assignment operator#10353

Merged
yhabteab merged 3 commits intomasterfrom
fix-string-move-constructor-and-assignment-operator
Mar 7, 2025
Merged

Fix string move constructor and assignment operator#10353
yhabteab merged 3 commits intomasterfrom
fix-string-move-constructor-and-assignment-operator

Conversation

@yhabteab
Copy link
Copy Markdown
Member

@yhabteab yhabteab commented Feb 27, 2025 •
edited by julianbrost
Loading

Warning

When backporting, also consider the following PR that fixes a problem in this PR:

The Icinga DB code performs heavyweight operations on certain STL containers,
primarily on std::vector<String>. Specifically, it inserts 2-3 new elements
at the beginning of a vector containing thousands of elements. Without this commit,
all the existing elements would be unnecessarily copied just to accommodate the new
elements at the front. By making this change, the compiler is able to optimize STL
operations like push_back, emplace_back, and insert, enabling it to prefer the
move constructor over copy operations, provided it is guaranteed that no exceptions
will be thrown.

@yhabteab yhabteab added the core/quality Improve code, libraries, algorithms, inline docs label Feb 27, 2025
@yhabteab yhabteab added this to the 2.15.0 milestone Feb 27, 2025
@yhabteab yhabteab requested a review from julianbrost February 27, 2025 17:15
@cla-bot cla-bot bot added the cla/signed label Feb 27, 2025
@Al2Klimov
Copy link
Copy Markdown
Member

Al2Klimov commented Mar 5, 2025

The idea itself LGTM. Another idea:

Change those std::vector<String> to std::vector<T> where each T is String or const char* to avoid malloc(3).

I mean, just look how many purely hardcoded "C string literals" we use in Icinga DB to assemble Redis cmds.

@julianbrost julianbrost added the consider backporting Should be considered for inclusion in a bugfix release label Mar 5, 2025
@julianbrost
Copy link
Copy Markdown
Member

julianbrost commented Mar 6, 2025

@yhabteab Can you please add some explanation to f15f99f why this is done (just like for the other commit)?

The idea itself LGTM. Another idea:

Change those std::vector<String> to std::vector<T> where each T is String or const char* to avoid malloc(3).

That's another idea in the sense that it would be an additional idea. If a std::vector<icinga::String> copies every string when it grows because its constructor/assign operators are implemented in a disadvantageous way whereas a std::vector<std::string> just moves the strings, that's a problem that should be fixed either way (and that fix is quite trivial).

@julianbrost julianbrost force-pushed the fix-string-move-constructor-and-assignment-operator branch from 79aade1 to f15f99f Compare March 6, 2025 11:13
@julianbrost
Copy link
Copy Markdown
Member

julianbrost commented Mar 6, 2025

Oops, didn't want to push that here. That force push should just have restored the original state of the PR.

@julianbrost
Copy link
Copy Markdown
Member

julianbrost commented Mar 6, 2025

I've written a test case that checks that the changes to the string operators have the desired effect: 79aade1

Feel free to have a look and include it in the PR (bonus points for something similar for the other change).

yhabteab and others added 2 commits March 6, 2025 13:02
@yhabteab
String: Mark move constructor & assignment op as noexcept
6a888e1 
The Icinga DB code performs intensive operations on certain STL containers,
primarily on `std::vector<String>`. Specifically, it inserts 2-3 new elements
at the beginning of a vector containing thousands of elements. Without this commit,
all the existing elements would be unnecessarily copied just to accommodate the new
elements at the front. By making this change, the compiler is able to optimize STL
operations like `push_back`, `emplace_back`, and `insert`, enabling it to prefer the
move constructor over copy operations, provided it is guaranteed that no exceptions
will be thrown.
@julianbrost @yhabteab
Add test that std::vector<icinga::String> uses move overloads
e308552
@yhabteab yhabteab force-pushed the fix-string-move-constructor-and-assignment-operator branch from f15f99f to f1b79f4 Compare March 6, 2025 12:04
@yhabteab
Copy link
Copy Markdown
Member Author

yhabteab commented Mar 6, 2025

@yhabteab Can you please add some explanation to f15f99f why this is done (just like for the other commit)?

Done!

Feel free to have a look and include it in the PR (bonus points for something similar for the other change).

Both done!

@yhabteab yhabteab force-pushed the fix-string-move-constructor-and-assignment-operator branch from f1b79f4 to 8735e6f Compare March 6, 2025 12:48
julianbrost
julianbrost requested changes Mar 6, 2025
View reviewed changes
@yhabteab yhabteab force-pushed the fix-string-move-constructor-and-assignment-operator branch from 8735e6f to 01a8bb9 Compare March 6, 2025 16:48
@yhabteab yhabteab requested a review from julianbrost March 6, 2025 16:49
@yhabteab
Value: Add a specialized rvalue reference of Get()
3e9292a 
The move `String(Value&&)` constructor tries to partially move `String`
values from a `Value` type. However, since there was no an appropriate
`Value::Get<T>()` implementation that binds to the requested move
operation, the compiler will actually not move the value but copy it
instead as the only available implementation of `Value::Get<T>()`
returns a const reference `const T&`. This commit adds a new overload
that returns a non-const reference and allows to optionally move the string
value of a Value type.
@yhabteab yhabteab force-pushed the fix-string-move-constructor-and-assignment-operator branch from 01a8bb9 to 3e9292a Compare March 7, 2025 09:21
@yhabteab yhabteab requested review from julianbrost and removed request for julianbrost March 7, 2025 09:22
@yhabteab yhabteab merged commit 35520b5 into master Mar 7, 2025
23 checks passed
@yhabteab yhabteab deleted the fix-string-move-constructor-and-assignment-operator branch March 7, 2025 11:50
@gbin2265
Copy link
Copy Markdown

gbin2265 commented Mar 8, 2025

For your information:

this is what I get when I try to compile to RHEL9 with mock and use the source fc41

Source file : icinga2-2.14.5+467.g206d7cda1-1741385012.fc41.src.rpm

Schermafbeelding 2025-03-08 125021

Does this have anything to do with this update?

@yhabteab
Copy link
Copy Markdown
Member Author

yhabteab commented Mar 10, 2025 •
edited
Loading

Does this have anything to do with this update?

Hi @gbin2265, thanks for reporting! Yes, that is caused by this PR! I have created a new PR that fixes that.

@julianbrost julianbrost mentioned this pull request Mar 10, 2025
Merged
@yhabteab yhabteab removed the consider backporting Should be considered for inclusion in a bugfix release label Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julianbrost julianbrost julianbrost approved these changes

Assignees

No one assigned

Labels

cla/signed core/quality Improve code, libraries, algorithms, inline docs

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

4 participants

@yhabteab @Al2Klimov @julianbrost @gbin2265