1
// VERSION 1
struct Range { int begin, end; };
inline Range getRange()
{
    int newBegin, newEnd;
    // do calculations
    return {newBegin, newEnd};
}
struct Test
{
    std::vector<Range> ranges;
    inline void intensive()
    {
        ranges.push_back(getRange());
        // or ranges.emplace_back(getRange());
        // (gives same performance results)
    }
};

// VERSION 2
struct Range { int begin, end; };
struct Test
{
    std::vector<Range> ranges;
    inline void intensive()
    {
       int newBegin, newEnd;
       // do calculations
       ranges.emplace_back(newBegin, newEnd);
    }
};

Version 2 is always faster than version 1.

Fact is, getRange() is used by multiple classes. If I were to apply version 2, there would be a lot of code duplication.

Also, I cannot pass ranges as a non-const reference to getRange(), as some other classes use a std::stack instead of a std::vector. I would have to create multiple overloads and have more code duplications.

Is there a common way/idiom to emplace the return value?

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
Vittorio Romeo
  • 90,666
  • 33
  • 258
  • 416
  • Try returning an r-value reference from the `getRange` function. – user123 Aug 23 '13 at 14:40
  • 4
    "Version 2 is always faster than version 1." -- and that assertion is based upon... what? – Xeo Aug 23 '13 at 14:43
  • 5
    @MohammadAliBaydoun: No, full stop, wind back and think again. – Xeo Aug 23 '13 at 14:44
  • @MohammadAliBaydoun: doesn't work. Crashes at runtime with only changing return type to `Range&&`. Also tried with `std::move`, same result. – Vittorio Romeo Aug 23 '13 at 14:46
  • @Xeo: repeated tests with full optimization settings both on clang++3.4 and g++4.8.1 in "real" code – Vittorio Romeo Aug 23 '13 at 14:46
  • 3
    @user1837009 NO. Returning an initializer_list is the moral equivalent of returning a reference. (it would also not help at all) – R. Martinho Fernandes Aug 23 '13 at 14:49
  • Regarding "*passing `ranges` as a non-const reference to `getRange()`*" => You **can** do that, using templates and SFINAE (so that you can automatically choose between `emplace_back` and `emplace` depending on their availability). – syam Aug 23 '13 at 14:49
  • @R.MartinhoFernandes: thanks, dont know that – user1837009 Aug 23 '13 at 14:50
  • @syam: Yeah, that was one of my ideas - I'll give it a try and if it's way uglier than expected I'll just duplicate the code – Vittorio Romeo Aug 23 '13 at 14:50
  • @VittorioRomeo The ugliness (there will be some, don't kid ourselves) will be limited to the `getRange` function, so IMHO it is worth the trouble: I'll teach you nothing telling you that code duplication is dangerous for long term maintenance since you have to manually update every instance of the code, which *invariably* leads to human errors at some point. – syam Aug 23 '13 at 14:55
  • @syam: [this is what I got so far](http://pastie.org/8262970), it works, but I'm sure it could be nicer. Any idea on how to improve it? – Vittorio Romeo Aug 23 '13 at 15:09
  • 1
    @Vittorio Well if you're going to overload like this, you don't really need all that stuff, you could just have two overloaded functions without the additional template classes (I assume `T` and `TItemArgs` are known and fixed in your real code, so you're just left with `TArgs`). What I had in mind was [something along those lines](http://stackoverflow.com/questions/257288/is-it-possible-to-write-a-c-template-to-check-for-a-functions-existence) to allow for *any* container that supports either `emplace` or `emplace_back`, eg. `queue/list/deque` if you ever happen to need them in the future. – syam Aug 23 '13 at 15:28
  • @syam: Having trouble understanding *and implementing* that solution. I changed `typeof` with `decltype`, and it seems to compile. However, how do I "apply" this solution? Do I need to use `std::enable_if`? – Vittorio Romeo Aug 23 '13 at 15:39
  • 1
    @VittorioRomeo I'm trying to put an example together but I'm having trouble because `emplace` and `emplace_back` are overloaded, so it silently fails the SFINAE test and never detects the function. Please give me a moment... ;) – syam Aug 23 '13 at 15:50
  • Did you try to *force* inlining? E.g. via the (g++ specific) `__attribute__((always_inline))` (not sure if that's sufficient, look at the assembler code) – dyp Aug 23 '13 at 16:56
  • (I'm referring to `inline Range getRange()`) – dyp Aug 23 '13 at 17:04
  • 1
    Can you generate a [sscce](http://sscce.org)? Something could be going on in your `// do calculations` and in `getRange()` that is messing with your performance. What if you do `int beg, end; std::tie(beg, end) = getRange()`, where `getRange` returns a `std::tuple`, in #2? – Yakk - Adam Nevraumont Aug 23 '13 at 18:18
  • 1
    Are the types in question *actually `int`s*? Is the `struct Range` really that simple? – Yakk - Adam Nevraumont Aug 23 '13 at 18:21

3 Answers3

4

Following our discussion in the comments about using SFINAE to allow emplacement on any type of container (whether it supports emplace or emplace_back), here is an example implementation.

You just need a way to detect whether emplace or emplace_back is available, and dispatch the call accordingly. For this purpose, we'll use SFINAE:

namespace detail
{
    template<typename T, typename... Args>
    auto emplace_impl(int, T& c, Args&&... pp)
        -> decltype(c.emplace_back(std::forward<Args>(pp)...))
    {
        return c.emplace_back(std::forward<Args>(pp)...);
    }

    template<typename T, typename... Args>
    auto emplace_impl(long, T& c, Args&&... pp)
        -> decltype(c.emplace(std::forward<Args>(pp)...))
    {
        return c.emplace(std::forward<Args>(pp)...);
    }
} // namespace detail

template<typename T, typename... Args>
auto emplace(T& c, Args&&... pp)
    -> decltype(detail::emplace_impl(0, c, std::forward<Args>(pp)...))
{
    return detail::emplace_impl(0, c, std::forward<Args>(pp)...);
}

Kudos to @DyP who provided this much nicer and shorter C++11 solution (see comments). The previous traits-based solutions (revisions 3 & 4) were a lot more verbose.


Using it is quite straightforward:

template<typename Container>
void test_emplace()
{
  Container c;
  emplace(c, 3);
}

int main()
{
  test_emplace<std::queue<int>>();
  test_emplace<std::stack<int>>();
  test_emplace<std::deque<int>>();
  test_emplace<std::list<int>>();
  test_emplace<std::vector<int>>();
}

I'll let you bridge the gap between my test_emplace() usage example and your actual code, but it shouldn't be too hard now. ;)

syam
  • 14,701
  • 3
  • 41
  • 65
  • 1
    *Please* check my answer [here](http://stackoverflow.com/a/9154394/500104) on how to properly check if you can invoke a function with certain parameters in C++11. – Xeo Aug 23 '13 at 16:49
  • 2
    "Since it is a requirement of standard containers that their items are `CopyConstructible`"... No, it isn't. `std::vector>` is fine. – R. Martinho Fernandes Aug 23 '13 at 16:51
  • 1
    Don't test for the existence of a member with a specific signature. Test for the validity of an expression (i.e. use `decltype(std::declval().emplace(std::declval()))`) – R. Martinho Fernandes Aug 23 '13 at 16:55
  • @R.MartinhoFernandes Yeah I'm looking at Xeo's post right now, trying to make sense of it... And of course you are right about the CopyConstructible part. I can't fix the answer right now (got urgent stuff to do *outside*) but I'll look into it as soon as I get back, in an hour or so. I guess just adding an overload for move-construction should be enough? – syam Aug 23 '13 at 16:58
  • 1
    Nah, forget the Copy/Move whatever constructible. If you make it `HasEmplace::value`, you can test directly for the exact arguments :) – R. Martinho Fernandes Aug 23 '13 at 17:03
  • @R.MartinhoFernandes Ah right this makes sense (and works like a charm)... Code fixed, thanks! – syam Aug 23 '13 at 18:04
  • @Xeo and R.MartinhoFernandes I realize testing the validity of an expression is preferred over testing the presence of a specific signature, but right now I don't really understand how to put it all together. I guess I'll have to digest it first before I can use it properly (namely, I don't understand yet how to use variadic template arguments along with `declval`, but I'll probably figure it out soon...). – syam Aug 23 '13 at 18:10
  • 1
    Does `std::declval()...` help out? (It would expand to something like `std::declval(), std::declval(), std::declval()`). – R. Martinho Fernandes Aug 23 '13 at 18:13
  • @R.MartinhoFernandes Ah yes, that was what I was looking for. Thanks again. I believe the implementation should be OK by now, thanks to you guys. :) – syam Aug 23 '13 at 18:31
  • 1
    I think this can be simplified. [Live example](http://coliru.stacked-crooked.com/view?id=5c1eff97cabfd7286adcc5b8bf9439be-25dabfc2c190f5ef027f31d968947336) – dyp Aug 23 '13 at 21:23
  • @DyP Looks nice. Please make it a full answer. ;) (actually I'm not quite comfortable with the ellipsis `...` at the end of the parameter list in the second overload, but I believe this is correct anyway -- it's just that those two sets of variadic arguments in the same function signature kinda make me nervous with respect to how the compiler deduces them when the template arguments are not explicitly given -- probably just my OCD acting up though) – syam Aug 23 '13 at 21:39
  • 1
    Actually, I don't believe the OP's performance issue has to do with `emplace` vs `push_back` (or emplace 2 `int`s vs 1 `struct`). It's very unlikely that one would be inlined but not the other. If it's not inlined, you need to put 2 `int`s on the stack in any case (no other copies as `getRange` can use NRVO). From the stack, they'll be copied into the destination container element. However, in his example, `getRange` is manually inlined in the second case but not in the first. I might have found a nice way to provide a generic `emplace` but that probably doesn't solve this performance issue. – dyp Aug 23 '13 at 21:56
  • 1
    If you're not comfortable with the ellipsis, you could [use a dispatcher](http://coliru.stacked-crooked.com/view?id=59949d30f5cf4c25089b2dd354edd79f-25dabfc2c190f5ef027f31d968947336). This also allows more general solutions with more than 2 overloads. Feel free to include any of this in your answer. – dyp Aug 24 '13 at 07:18
  • @DyP I included the second one. Thank you a lot (and everyone else too), I learned at least as much as OP here! – syam Aug 24 '13 at 14:11
3

Here is a way to you can pass the code to emplace into GetRange without knowing what it is you are emplacing into:

template<typename Emplacer>
void GetRange( Emplacer emplace ) {
  int beg, end;
  // ...
  emplace( beg, end );
}

std::vector<Range> ranges;
inline void intensive()
{
   GetRange( [&]( int b, int e ) {
     ranges.emplace_back( b, e );
   } );
}
Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • Nice one, +1. Might be a bit cumbersome to use a lambda at every call site though, but I still like the design. – syam Aug 23 '13 at 18:37
1

No, you are constructing with getRange() where as emplace_back has the construction done in the vector.

Paul Evans
  • 27,315
  • 3
  • 37
  • 54
  • I see. I wish the compiler could (or would) make these optimizations on its own. I can only think about creating a common interface for both `std::vector` and `std::stack` emplacement and passing the containers as non-const reference, but I guess a little code duplication is better. – Vittorio Romeo Aug 23 '13 at 14:49
  • [This is what I got so far](http://pastie.org/8262970), it works, but I'm sure it could be nicer. Any idea on how to improve it? – Vittorio Romeo Aug 23 '13 at 15:10