15

I'm in the process of updating a codebase that is currently using a custom equivalent of std::variant to C++17 .

In certain parts of the code, the variant is being reset from a known alternative, so the class provides a method that asserts that index() is at a current value, but still directly invokes the proper destructor unconditionally.

This is used in some tight inner loops, and has (measured) non-trivial performance impact. That's because it allows the compiler to eliminate the entire destruction when the alternative in question is a trivially destructible type.

At face value, it seems to me that I can't achieve this with the current std::variant<> implementation in the STL, but I'm hoping that I'm wrong.

Is there a way to accomplish this that I'm not seeing, or am I out of luck?

Edit: as requested, here's a usage example (using @T.C's example as basis):

struct S {
    ~S();
};

using var = MyVariant<S, int, double>;

void change_int_to_double(var& v){
  v.reset_from<1>(0.0);
}

change_int_to_double compiles to effectively:

@change_int_to_double(MyVariant<S, int, double>&)
  mov qword ptr [rdi], 0       // Sets the storage to double(0.0)
  mov dword ptr [rdi + 8], 2   // Sets the index to 2

Edit #2

Thanks to various insight from @T.C., I've landed on this monstrosity. It "works" even though it does violate the standard by skipping a few destructors. However, every skipped destructor is checked at compile-time to be trivial so...:

see on godbolt: https://godbolt.org/g/2LK2fa

// Let's make sure our std::variant implementation does nothing funky internally.
static_assert(std::is_trivially_destructible<std::variant<char, int>>::value, 
          "change_from_I won't be valid");

template<size_t I, typename arg_t, typename... VAR_ARGS>
void change_from_I(std::variant<VAR_ARGS...>& v, arg_t&& new_val) {
    assert(I == v.index());

    // Optimize away the std::get<> runtime check if possible.
    #if defined(__GNUC__) 
      if(v.index() != I) __builtin_unreachable();
    #else
      if(v.index() != I) std::terminate();
    #endif


    // Smart compilers handle this fine without this check, but MSVC can 
    // use the help.
    using current_t = std::variant_alternative_t<I, std::variant<VAR_ARGS...>>;
    if(!std::is_trivially_destructible<current_t>::value) {
        std::get<I>(v).~current_t();
    }
    new (&v) var(std::forward<arg_t>(new_val));
}
  • by chance are all the types in the variant trivially destructible? – kmdreko Sep 11 '17 at 03:55
  • @vu1p3n0x It's kinda squirreled away deep within a template hierarchy, so the answer is: sometimes yes, sometimes no. I haven't tested wether the compiler handles the yes case or not, but it doesn't matter as I want it to work when it's mixed. –  Sep 11 '17 at 04:03
  • "*the variant is being reset from a known alternative*" What does that mean, exactly? Can you provide code using your old type that demonstrates this? – Nicol Bolas Sep 11 '17 at 04:20
  • 1
    Something like https://godbolt.org/g/8zypv4? – T.C. Sep 11 '17 at 04:21
  • 1
    @T.C. No, because there's still a runtime check, In my version, the `cmp` and `jne` are not there. (Though I will say that I am impressed the compiler pulled this off, it might be close enough, if not ideal) –  Sep 11 '17 at 04:26
  • @NicolBolas Updated the question with a specific example. –  Sep 11 '17 at 04:30
  • 6
    GCC gives you what you want if you use `__builtin_unreachable()` instead of `terminate()`. Clang's codegen is terrible with that though. – T.C. Sep 11 '17 at 04:31
  • 1
    @T.C. Oh Wow! That's awesome, I just have to get clang and MSVC to play ball, but this is encouraging enough for me to proceed. Thanks! –  Sep 11 '17 at 04:36

1 Answers1

7
#include <variant>
struct S {
    ~S();
};
using var = std::variant<S, int, double>;

void change_int_to_double(var& v){
    if(v.index() != 1) __builtin_unreachable();
    v = 0.0;
}

GCC compiles the function down to:

change_int_to_double(std::variant<S, int, double>&):
  mov QWORD PTR [rdi], 0x000000000
  mov BYTE PTR [rdi+8], 2
  ret

which is optimal. Clang's codegen, OTOH, leaves much to be desired, although it isn't too bad if you use std::terminate() (the equivalent of an assertion) rather than __builtin_unreachable():

change_int_to_double(std::__1::variant<S, int, double>&): # @change_int_to_double(std::__1::variant<S, int, double>&)
  cmp dword ptr [rdi + 8], 1
  jne .LBB0_2
  mov qword ptr [rdi], 0
  mov dword ptr [rdi + 8], 2
  ret
.LBB0_2:
  push rax
  call std::terminate()

MSVC...let's not talk about MSVC.

T.C.
  • 133,968
  • 17
  • 288
  • 421
  • 1
    For reference, yeah, MSVC (2017) actually maintains a call to `std::_Variant_base::_Reset` even with the most aggressive optimization options when using the `std::terminate` approach. Which is... not great... –  Sep 11 '17 at 07:24
  • I upvoted the answer but won't accept it just yet. I would much rather have a solution that performs this in code (like I used to have), rather than hoping the compiler optimizes it for me. I do understand that may not be possible right now though... –  Sep 11 '17 at 18:00
  • @Frank Hmm, there's always the check-trivial-destructible-and-placement-new-over-the-thing approach if you want to go that way... – T.C. Sep 11 '17 at 18:13
  • I would need a way to set the just the index in order to be able to do that. –  Sep 11 '17 at 18:16
  • @Frank No, you placement new a whole new `variant`. – T.C. Sep 11 '17 at 18:21
  • improving on that a bit: https://godbolt.org/g/VA4U9d. This is walking a verrrrrrry thin line, and is technically invalid, but "safe enough" with the static_assert in place (I think). –  Sep 11 '17 at 19:02
  • 1. If you want to play a little more by the books, just use plain assignment if the current alternative isn't trivially destructible. Might be a little more expensive, but you are calling a dtor anyway. 2. In general, skipping dtors is fine as long as you don't depend on their side effects. 3. The dtor of `std::variant` [must be trivial](https://timsong-cpp.github.io/cppwp/variant.dtor#2) if every alternative is trivially destructible. – T.C. Sep 11 '17 at 19:32