Avoid string copies when string is consumed
Thomas1664 opened this issue · 4 comments
Functions like ClientImpl::ClientImpl(...) or ClientImpl::Get(...) take strings by const& although the strings are used to initialize a (member) variable. In the case of ClientImpl::Get(...), this creates at least one copy of the string (assuming nested calls are inlined) or in the worst case five copies of the string (without inlining).
The best way to fix this would be using universal references via templates. This would allow perfect forwarding of the arguments without potentially constructing and copying temporaries.
The "best" is arguable; the usual way of dealing with this would be by using the value + move pattern. Sure, using forwarding references could be the ideal way from a programmatical standpoint, but it would increase compilation time while hindering readability. Adding lvalue + rvalue overloads would also be a possibility, but leads to code duplication and clutters the type's interface. Passing by value & moving regroups both those situations, and while it does create a temporary, it's the most straightforward way and is often advised in such cases.
In any case, I do agree with you about these unnecessary copies. I'd like to try making some changes in the near future, this would be one of those.
In any case, I do agree with you about these unnecessary copies. I'd like to try making some changes in the near future, this would be one of those.
I can also open a PR for this although I don't think I can put everything in one PR. I guess at least for the internal interface this should be fine? I just wanted to ask beforehand which approach you prefer and if you would have been fine with universal references.
As a side note: Is it possible to reduce the number of overloads for the internal interface, e.g. ClientImpl by defaulting some of the arguments?
Just in case, I have no affiliation with this repo, it's pretty much the first time I've come here; I just came across your issue right after you posted it 😄
Putting universal references for this can seriously hurt the compilation time and readability, I really doubt it'd be a good idea to do that. But after having taken a more detailed look, the chain of functions would result in so many consecutive moves that a single copy might be preferable; although it's already done for some arguments (e.g. ContentProvider, which is an std::function), so...
As a side note: Is it possible to reduce the number of overloads for the internal interface, e.g.
ClientImplby defaulting some of the arguments?
Definitely, that's what I also started to do 👍 (ClientImpl::Head(), for instance)
@Thomas1664 thanks for your suggestion. At this point, I am not planning to make such improvement unless it causes an obvious performance bottle neck.