9

Just for fun, I have implemented the simplest sorting algorithm imaginable:

template<typename Iterator>
void treesort(Iterator begin, Iterator end)
{
    typedef typename std::iterator_traits<Iterator>::value_type element_type;

    // copy data into the tree
    std::multiset<element_type> tree(begin, end);

    // copy data out of the tree
    std::copy(tree.begin(), tree.end(), begin);
}

It's only about 20 times slower than std::sort for my test data :)

Next, I wanted to improve the performance with move semantics:

template<typename Iterator>
void treesort(Iterator begin, Iterator end)
{
    typedef typename std::iterator_traits<Iterator>::value_type element_type;

    // move data into the tree
    std::multiset<element_type> tree(std::make_move_iterator(begin),
                                     std::make_move_iterator(end));
    // move data out of the tree
    std::move(tree.begin(), tree.end(), begin);
}

But this did not affect the performance in a significant way, even though I am sorting std::strings.

Then I remembered that associative containers are constant from the outside, that is, std::move and std::copy will do the same thing here :( Is there any other way to move the data out of the tree?

fredoverflow
  • 256,549
  • 94
  • 388
  • 662
  • Can you give us some more info on what strings you are sorting? Could you perhaps post your testing code for us to play with? – templatetypedef Jan 20 '13 at 20:54
  • It looks like you're trying to optimize something which you know could be optimized much more by simply using `qsort`. What's the purpose of doing that? Learning about move semantics? – svick Jan 20 '13 at 20:57
  • 2
    @svick Yeah, my primary question is: how do I move elements out of an associative container if I don't need them in there anymore? I'm sure that general question transcends my stupid Treesort example :) I modified the title of the question and removed the allocator bit, thank you. – fredoverflow Jan 20 '13 at 20:58
  • I suspect the answer is you can't -- there doesn't seem to be a pop-and-return. Making that exception safe would probably be as tricky, if not more so, than on a `std::vector`. – Yakk - Adam Nevraumont Jan 20 '13 at 21:01
  • You can't, but Howard Hinnant worked on an extension (I don't think there was an official paper but I could be wrong) that lets you extract a node from the container, and then you can move its value. So maybe in a few years, with a newer standard... – Marc Glisse Jan 20 '13 at 21:15
  • Even if you could move the elements your `treesort` would still allocate memory for the `multiset` nodes, rather than sorting in place. – Jonathan Wakely Jan 21 '13 at 00:10

3 Answers3

8

std::set and std::multiset only provide const access to their elements. This means you cannot move something out of the set. If you could move items out (or modify them at all), you could break the set by changing the sort order of the items. So C++11 forbids it.

So your attempt to use the std::move algorithm will just invoke the copy constructor.

Nicol Bolas
  • 449,505
  • 63
  • 781
  • 982
4

I believe you could make a custom allocator for the multiset to use (3rd template argument) which actually moves the elements in it's destroy method back to the user's container. Then erase each element in the set and during its destruction it should move your string back to the original container. I think the custom allocator would need to have 2 phase construction (pass it the begin iterator passed to yourtreesort function to hold as a member, but not during construction because it has to be default constructible).

Obviously this would be bizarre and is a silly workaround for not having a pop method in set/multiset. But it should be possible.

David
  • 27,652
  • 18
  • 89
  • 138
0

I like Dave's idea of a freaky allocator that remembers the source of each move constructed object and automatically moves back on destruction, I'd never thought of doing that!

But here's an answer closer to your original attempt:

template<typename Iterator>
void treesort_mv(Iterator begin, Iterator end)
{
    typedef typename std::iterator_traits<Iterator>::value_type element_type;

    // move the elements to tmp storage
    std::vector<element_type> tmp(std::make_move_iterator(begin),
                                  std::make_move_iterator(end));
    // fill the tree with sorted references
    typedef std::reference_wrapper<element_type> element_ref;
    std::multiset<element_ref, std::less<element_type>> tree(tmp.begin(), tmp.end());

    // move data out of the vector, in sorted order
    std::move(tree.begin(), tree.end(), begin);
}

This sorts a multiset of references, so they don't need to be moved out of the tree.

However, when moving back into the original range the move assignments are not necessarily safe for self-assignment, so I moved them into a vector first, so that when re-assigning them back to the original range there will not be self-assignments.

This is marginally faster than your original version in my tests. It probably loses efficiency because it has to allocate the vector as well as all the tree nodes. That and the fact that my compiler uses COW strings so moving isn't much faster than copying anyway.

Jonathan Wakely
  • 166,810
  • 27
  • 341
  • 521