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

Prevent copying by using references, also fixes warnings #4938

Merged
merged 1 commit into from
May 7, 2024

Conversation

rsf92
Copy link
Contributor

@rsf92 rsf92 commented Apr 29, 2024

I couldn't run the tests (don't know how) but I think this changes ought to be safe

@geoffthemedio
Copy link
Member

geoffthemedio commented Apr 29, 2024

Most of these are probably pointless or actually counter productive. Eg. a structured binding to two int variables. There should be no substantial extra copying occurring since they are small built in trivial types. If the references are implemented as a separately allocated pointer or multiple pointers, then that would be as big or bigger (8 bytes or more if multiple ponters are used) than the two ints. In practice I'd expect the generated code to be the same anyway.

Cases where the bound object is something substantially bigger than a pointer are different, though.

@rsf92
Copy link
Contributor Author

rsf92 commented Apr 29, 2024

Well, it reported the warning and now it doesn't. I figured that somebody with more knowledge than me could potentially see some hiccups with the change.

Maybe it's better to close this PR

@Vezzra Vezzra added category:tweak The PR contains insignificant code changes, like code style grooming or value tweaking. component:internal The Issue/PR deals with any project component that has no explicit `component` label. labels May 1, 2024
@jlumb-gc
Copy link

jlumb-gc commented May 6, 2024

There should be no substantial extra copying occurring since they are small built in trivial types. If the references are implemented as a separately allocated pointer or multiple pointers, then that would be as big or bigger (8 bytes or more if multiple ponters are used) than the two ints. In practice I'd expect the generated code to be the same anyway.

Agreed that there won't be a whole tonne of extra work done, but there will likely be some extra work done which will make the generated instructions slightly less efficient. Iterating over the map's elements by value causes the compiler to copy the pair's elements onto the stack even at a high optimisation level - even for trivial types.
If we instead iterate via const-ref it seems that the compiler can elide this copy and optimize its algo to run directly on the container in-place.

Example with diff on Clang 15 with -O3 using C++20 (using a std::map instead of flat_map. stdout used to ensure our example data / instructions aren't optimised out.)
https://godbolt.org/z/hMYEddEod

@geoffthemedio geoffthemedio merged commit 3ebebdb into freeorion:master May 7, 2024
15 checks passed
@geoffthemedio
Copy link
Member

reasonable evidence of benefit, so OK

@Vezzra Vezzra added this to the v0.5.1 milestone May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tweak The PR contains insignificant code changes, like code style grooming or value tweaking. component:internal The Issue/PR deals with any project component that has no explicit `component` label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants