Page MenuHomeWildfire Games

Rely on copy-elision / return-value-optimization for lobby string getters
Needs ReviewPublic

Authored by elexis on Nov 9 2018, 5:09 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Passing a mutable string reference for a trivial string getter seems like unneeded complexity. D1667 does the same to the JS::Values.
https://en.wikipedia.org/wiki/Copy_elision#Return_value_optimization

Test Plan

Test whether we really don't return pointers and that RVO really avoids the copies.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6437
Build 10663: Vulcan BuildJenkins
Build 10662: arc lint + arc unit

Event Timeline

elexis created this revision.Nov 9 2018, 5:09 PM
Vulcan added a subscriber: Vulcan.Nov 9 2018, 6:28 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/771/

@vladislavbelov are most compilers really smart enough to use copy elision for strings here as widely claimed on the internet?
(Some places use one pattern, other places use the other pattern, we should make up our mind what we want, otherwise it sounds contradictory if we claim there to be good reasons for both.)

In D1668#66468, @elexis wrote:

@vladislavbelov are most compilers really smart enough to use copy elision for strings here as widely claimed on the internet?
(Some places use one pattern, other places use the other pattern, we should make up our mind what we want, otherwise it sounds contradictory if we claim there to be good reasons for both.)

Smart, but the (N)RVO isn't guaranteed. Only since C++17.

I agree, that we should have the same style, but we may have different cases. We can easily return a small std::string, but a huge object can't be allocated on a stack.

source/lobby/XmppClient.cpp
958

Ternary operator here and below.

elexis marked an inline comment as done.Wed, Nov 21, 10:51 PM

Smart, but the (N)RVO isn't guaranteed. Only since C++17.

https://de.cppreference.com/w/cpp/language/copy_elision looks more like C++11, only some other features C++17?

I agree, that we should have the same style, but we may have different cases. We can easily return a small std::string, but a huge object can't be allocated on a stack.

At least for strings getters like here it seems useless indirection.

But what about JS::Values, like in D1667 - do they also work with copy elision, i.e.

- void XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface, JS::MutableHandleValue ret)
+ JS::Value XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface)

If copy elision doesnt work with JS::Value, we might eventually change JS::Value JSI_VisualReplay::GetReplays(ScriptInterface::CxPrivate* pCxPrivate, bool compareFiles) and so forth, so that people chose the better variant for new code?

source/lobby/XmppClient.cpp
958

I once had to do revert of a ternary in a getter because the if return return pattern should be used everywhere. So the else keyword should go I suppose

In D1668#66513, @elexis wrote:

Smart, but the (N)RVO isn't guaranteed. Only since C++17.

https://de.cppreference.com/w/cpp/language/copy_elision looks more like C++11, only some other features C++17?

Yeah, compilers started optimize these cases a long time ago. But there're many nuances.

But what about JS::Values, like in D1667 - do they also work with copy elision, i.e.

- void XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface, JS::MutableHandleValue ret)
+ JS::Value XmppClient::GUIGetGameList(const ScriptInterface& scriptInterface)

js/Value.h from SpiderMonkey:

* - Note that JS::Value is 8 bytes on 32 and 64-bit architectures. Thus, on
*   32-bit user code should avoid copying jsval/JS::Value as much as possible,
*   preferring to pass by const Value&.

std::string is ~24bytes usually, so it's not worse to return JS::Value .

So I'd prefer to do some measurements/perf-tests.