Modify

Ticket #7718 (closed Patches: fixed)

Opened 17 months ago

Last modified 12 months ago

Basic rvalue and C++11 support (part 2)

Reported by: apolukhin Owned by: ebf
Milestone: Boost 1.53.0 Component: variant
Version: Boost 1.52.0 Severity: Optimization
Keywords: Cc: antoshkka@…

Description

This patch adds template <class T> variant(T&&) constructor to Boost.Variant for compilers with rvalue references support (rvalue emulation via Boost.Move is not used for compatability reasons).

For more info see ticket #7620

Attachments

rvalues2.patch Download (8.8 KB) - added by apolukhin 17 months ago.
rvalue_test.cpp Download (6.5 KB) - added by apolukhin 17 months ago.
Updated tests (now tests variant usage with move-only containers)

Change History

comment:1 Changed 17 months ago by apolukhin

'variant_reference_test' currently fails with this patch.

Changed 17 months ago by apolukhin

comment:2 Changed 17 months ago by apolukhin

Now compiles and passes 'variant_reference_test', but sigfaults at 'test3'

Changed 17 months ago by apolukhin

Updated tests (now tests variant usage with move-only containers)

comment:3 Changed 17 months ago by apolukhin

(In [81617]) Basic rvalues and C++11 support part 2 (refs #7718 , all bugs from patch were fixed)

comment:4 Changed 17 months ago by apolukhin

(In [81652]) Refs #7718 : Workaroung GCC-4.7.2 internal compiler error More functions marked with BOOST_NOEXCEPT Added move constructors and move assignment operators to recursive_wrapper

comment:5 Changed 17 months ago by apolukhin

  • Status changed from new to closed
  • Resolution set to fixed

(In [81793]) Merge variant from trunk:

  • Fix #7718 (move constructor from template type added)
  • More tests and minor bugfixes
  • Deprecated macros replaced with new ones (thanks to Marshall Clow)

comment:6 Changed 16 months ago by djowel

  • Status changed from closed to reopened
  • Resolution fixed deleted

Hi,

I just looked at the current state of variant and noticed that the implementation of recursive_variant's move ctor seems to be not as optimal as I hoped. The current implementation as written is:

recursive_wrapper<T>::recursive_wrapper(recursive_wrapper&& operand)

: p_(new T( detail::variant::move(operand.get()) ))

{ }

Unless I am mistaken, I think the heap allocation is not needed and the target should simply grab the pointer and mark the source pointer as 0 (with additional checks for 0 in the dtor):

template <typename T> recursive_wrapper<T>::recursive_wrapper(recursive_wrapper&& operand)

: p_(operand.release())

{ }

template <typename T> T* recursive_wrapper<T>::release() {

T* ptr = p_; p_ = 0; return ptr;

}

template <typename T> recursive_wrapper<T>::~recursive_wrapper() {

if (p_)

boost::checked_delete(p_);

}

The tests are running just fine (tested with gcc, msvc) with this (more optimal) implementation. I also run the Spirit test suite (Spirit makes heavy use of variant).

Thoughts? I can commit this patch if it is acceptable.

comment:7 Changed 16 months ago by anonymous

Update. Just checked, the check for 0 is not needed. 0 is valid for checked_delete just like plain delete.

comment:8 Changed 15 months ago by apolukhin

  • Status changed from reopened to closed
  • Resolution set to fixed
  • Milestone changed from To Be Determined to Boost 1.53.0

This issue was discussed in  mainling lists. If in short we have to choose between thre solutions:
1) leave as is (slow move construction)
2) allow empty state (fast, but breaks users code and variant guarantees)
3) after move, do lazy default construction of value on request (fast, but adds default construction requirement for type and functions get() and get_ptr() may throw)

There were more votes for solution 1.

comment:9 Changed 15 months ago by djowel

  • Status changed from closed to reopened
  • Resolution fixed deleted

There is no such vote! 2 is the current consensus, FYI. I will reopen this. This is not a closed issue, not especially the way you concluded it to be.

comment:10 Changed 15 months ago by djowel

BTW, you are wrong about 2. The way Peter Dimov outlines the solution does not break users code and variant guarantees. Have you read Peter Dimov's solution?

Last edited 15 months ago by djowel (previous) (diff)

comment:11 Changed 15 months ago by djowel

Let me quote in case you missed it:

On 1/11/13 11:02 PM, Paul Smith wrote:

On Fri, Jan 11, 2013 at 4:18 PM, Peter Dimov <lists@…> wrote:

If the variant could be empty, or equivalently, if the variant always contained a type that can be reliably default-constructed (without an exception) such as int, the problems with assignment (both copy- and move-) and with move construction do not occur, because the target can be left holding the int.

So, one option is to enable move only in this case.

That's the first constructive solution so far. Something like:

template <typename T> struct is_legitimate_move_target specialize me

: has_trivial_constructor<T> {};

Paul, I agree!

Peter, I like the direction this is heading to. I really appreciate your interest in this.

comment:12 Changed 15 months ago by apolukhin

Sometimes I really miss a smiley that hits its head to the wall...

Variant is not a recursive_wrapper, recursive_wrapper is not a variant. recursive_wrapper may be used without a variant. recursive_wrapper currently can not hold an empty state. All the solutions above (1, 2 and 3) are for recursive_wrapper only.

Please, refer to the sources and documentation of variant. There you can find that solution of Peter Dimov already implemented...

If you have some other ideas for optimizing variant, please tell me about them short and clear. Otherwise, please close the ticket.

comment:13 Changed 15 months ago by djowel

:-) There you go (smiley). I'm sorry if I overreacted. This issue is very important for me as I've invested heavily on variant and I expect an optimal solution to this problem.

I know the code of variant and recursive_variant. This patch is about variant. recursive_wrapper happens to be an integral part of variant. No, the current code does not implement Peter Dimov's suggestion about move yet. If you think it does, then please point me to the location where this is implemented.

The idea is simplified with Peter's note:

<quote>

No, I'm suggesting that move (in the presence of recursive_wrappers) can be enabled only when there is at least one type that is nothrow-default-constructible, regardless of whether it's current.

typedef variant<int, recursive_wrapper<foo>> V;

V v1( std::move(v2) );

This move-constructs v1 from v2 and leaves int() into v2.

</quote>

So, this is a special case handling for recursive_variant. Depending on the implementation, there might be a friend relationship with variant in recursive_wrapper, allowing it to grab its contents and place it in the destination.

Note: I'll just check the the first type. Variant's design somehow makes the first type important. It's the one that's default constructed and it's (almost always) a nothrow-default-constructible type.

Tell me if you need help in implementing this. I know my way around variant.

comment:14 Changed 15 months ago by apolukhin

Now I've got the idea :-) . Yes, it is not currently implemented and will give a good speed boost.
But in example above, won't the user be confused by v2 holding an int (instead of recursive_wrapper)? This proposal looks not much better than the first proposal, but requires more code to implement and adds 'friend' relationship (which is not nice).

Maybe a more correct solution would be to create an additional class nullable_recursive_wrapper/lazy_recursive_wrapper, and possibly mark recursive_wrapper as a deprecated one? Or just use an unique_ptr/shared_ptr instead of it?

comment:15 Changed 15 months ago by djowel

No. As I said in the mailing list, this should happen only in the 0.0000001% of the cases. In 99.9999999% of the cases, when you move something, the next thing that will happen is that it will be destructed. Please read the extensive exchange on this matter in the list. Only in very rare occasions (dare I say reckless cases) will someone want to do something on a moved-from object after a move. In that case, documentation should be sufficient to make this clear.

Last edited 15 months ago by djowel (previous) (diff)

comment:16 follow-up: ↓ 17 Changed 15 months ago by djowel

BTW, that "No" is in reply to "won't the user be confused by v2 holding an int (instead of recursive_wrapper)?".

Perhaps there's a way to implement it without friend access, but I'm not sure. Anyway, the extra code is mostly TMP code for checking the the "blank" type.

nullable_recursive_wrapper is a good idea, but there's this extra check for null on all access that makes it less optimal. And that null check will be there to make the 0.0000001% of the cases happy. I don't know what you mean by lazy_recursive_wrapper. Same thing? As for unique/shared_ptr, AFAICT, they won't work as plug-in replacements.

comment:17 in reply to: ↑ 16 Changed 15 months ago by apolukhin

First of all, I also dislike that 'new' call in move constructor, but I just do not see a better solution. Let me explain:

Replying to djowel:

BTW, that "No" is in reply to "won't the user be confused by v2 holding an int (instead of recursive_wrapper)?".

Perhaps there's a way to implement it without friend access, but I'm not sure. Anyway, the extra code is mostly TMP code for checking the the "blank" type.

nullable_recursive_wrapper is a good idea, but there's this extra check for null on all access that makes it less optimal. And that null check will be there to make the 0.0000001% of the cases happy.

I'm trying to point, that solution that makes variant change its internal type is as good/bad for user, as a solution with nulling the pointer of recursive_wrapper. But nulling the pointer is much more easy to implement.

Here is some comparison of solutions (as I see it):

Nulling pointer after move constructor:
1) fast and efficient
2) triggers an assert when user tries to reuse moved object
3) must have a BOOST_ASSERT in get_pointer() finction
4) easy to implement
5) optimization will be used even if varinat has no type with trivial default constructor

Make varinat change its internal type:
1) fast and efficient
2) obscures user, when he tries to reuse moved object
3) requires a few more assignments in variant for which_ and maybe for trivial type construction
4) hard to implement
5) optimization will be used only if varinat has no type with trivial default constructor

I don't know what you mean by lazy_recursive_wrapper. Same thing?

It is an implementation, that after move does a lazy default construction of value on request.

comment:18 follow-up: ↓ 19 Changed 15 months ago by djowel

To be honest, I still prefer nulling the pointer. However, people seem to agree that that will ultimately place a precondition on recursive_variant (which is not good on extremely rare cases). Me? I don't care much about that. IMO, it's just a matter of documentation that people should simply not use a moved-from object.

The solution Peter suggested is a compromise. You do not need any asserts (which is present only on debug builds) and the variant's never-empty guarantee is not violated. If you are not happy with it, then you can probably ask for a vote in the Boost list. I'd personally vote for nulling the pointer. I'm sure many will too.

Bottom line: Either way is fine by me. I don't care which way we choose as long as we don't choose the "leave it as is" route.

comment:19 in reply to: ↑ 18 Changed 15 months ago by apolukhin

Replying to djowel:

To be honest, I still prefer nulling the pointer.

Nulling a pointer looks like a less ugliest solution. I`ll start a vote in the Boost list (and hope this time, nulling a pointer will win).

comment:20 Changed 12 months ago by apolukhin

(In [84104]) Update docs of Boost.Variant. Add advice about recursive_wrapper performance (refs #7718)

comment:21 Changed 12 months ago by apolukhin

As a result of discussion at mailing list - adding an empty state to recursive_wrapper is not a good idea (unfortunately). Updated documentation contains note about that and an advice for achieving better performance.

comment:22 Changed 12 months ago by apolukhin

  • Status changed from reopened to closed
  • Resolution set to fixed

(In [84105]) Merge from trunk:

  • Update docs of Boost.Variant. Add advice about recursive_wrapper performance (fixes #7718)
View

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.