1
\$\begingroup\$

Please review the following piece of code. The class implements a light weight wrapper on top of boost::container::vector. I am not getting the expected performance — nowhere comparable to std::vector. The vector is functionally correct but i feel there is some room for improvement, especially in the constructor methods. The wrapper is implemented for our abstraction to switch between different implementations. In our project we use lot of floating point numbers and bools as vector elements.

    #include <stdint.h>
    #include <errno.h>
    #include <malloc.h>
    #include <math.h>
    #include <pthread.h>
    #include <stdarg.h>
    #include <stdbool.h>
    #include <stdio.h>
    #include <stdlib.h>
    #include <string.h>
    #include <sys/mman.h>
    #include <sys/syscall.h>
    #include <sys/types.h>
    #include <sys/ipc.h>
    #include <sys/shm.h>
    #include <thread>
    #include <mutex>
    #include <unistd.h>
    #include <boost/interprocess/managed_mapped_file.hpp>
    #include <boost/interprocess/managed_shared_memory.hpp>
    #include <boost/align/aligned_allocator.hpp>
    #include <boost/align/aligned_allocator_adaptor.hpp>
    #include <boost/pool/pool_alloc.hpp>
    #include <iostream>
    #include <atomic>
    #include <inttypes.h>
    #include <dlfcn.h>
    #include <vector>
    #include <boost/preprocessor/stringize.hpp>
    #include <boost/container/scoped_allocator.hpp>
    #include <scoped_allocator>
    #include <memory>
    #include <iterator>
    #define UNW_LOCAL_ONLY
    #include <libunwind.h>
    #ifndef BOOST_DISABLE_ASSERTS
    #define BOOST_DISABLE_ASSERTS
    #include <boost/container/small_vector.hpp>
    #include <boost/container/vector.hpp>
    #include <boost/container/map.hpp>
    #endif

    #ifdef USE_CUSTOM_VECTOR
    #ifndef DEFAULT_SMALL_VECTOR_LENGTH
    #define DEFAULT_SMALL_VECTOR_LENGTH 8
    #endif
    #include <type_traits>
    #include <memory>
    #include <algorithm>
    #include <stdexcept>
    #include <iterator>
    #include <iostream>
    #include <map>
    #include <mutex>
    #include <typeinfo>
    #include <typeindex>

    template<typename T, class Allocator = std::allocator<T>>
    class customVector
    {
        public:
            boost::container::vector<T, Allocator> *internal = nullptr;
boost::container::vector<T, Allocator> &_internal = *internal;
            using value_type        = typename boost::container::vector<T>::value_type;
            using reference         = typename boost::container::vector<T>::reference;
            using const_reference   = typename boost::container::vector<T>::const_reference;
            using pointer           = typename boost::container::vector<T>::pointer;
            using const_pointer     = typename boost::container::vector<T>::const_pointer;
            using iterator          = typename boost::container::vector<T>::iterator;
            using const_iterator    = typename boost::container::vector<T>::const_iterator;
            using difference_type   = typename boost::container::vector<T>::difference_type;
            using size_type         = typename boost::container::vector<T>::size_type;
            using reverse_iterator  = boost::container::reverse_iterator<iterator>;
            using const_reverse_iterator = boost::container::reverse_iterator<const_iterator>;

            customVector(void)
            {
                assert(!_internal.size());
                return;
            }

            template <typename Iterator>
            customVector(Iterator first, Iterator last)
            {
                assert(!_internal.size());
                _internal.assign(first, last);
                return;
            }

            customVector(size_type capacity)
            {
                assert(!_internal.size());
                _internal.resize(capacity);
                return;
            }

            customVector(size_type n, const value_type &val)
            {
                assert(!_internal.size());
                _internal.reserve(n);
                while(n--) _internal.push_back(val);
                return;
            }

            customVector(const std::initializer_list<T> list)
            {
                assert(!_internal.size());
                std::copy(list.begin(), list.end(), std::back_inserter(_internal));
                return;
            }

            customVector(customVector const& copy)
            {
                assert(!_internal.size());
                std::copy(_internal.begin(), _internal.end(), std::back_inserter(copy._internal));
                return;
            }

            customVector(customVector&& move) noexcept
                {
                    assert(!_internal.size());
                    _internal = std::move(move._internal);
                    return;
                }

            ~customVector()
            {
                _internal.clear();
                return;
            }

            inline customVector& operator=(customVector const& copy) { _internal = copy._internal; return *this; }
            inline customVector& operator=(customVector&& move) noexcept { _internal = std::move(move._internal); return *this; }
            inline customVector& operator=(std::initializer_list<T> list)
            {
                std::copy(list.begin(), list.end(), std::back_inserter(_internal));
                return *this;
            }
            inline void swap(customVector& other) noexcept { _internal.swap(other._internal); }
            inline size_type           size() const                        { return _internal.size(); }
            inline bool                empty() const                       { return _internal.empty(); }
            inline reference           at(size_type index)                 { return _internal.at(index); }
            inline const_reference     at(size_type index) const           { return _internal.at(index); }
            inline reference           operator[](size_type index)         { return (_internal[index]); }
            inline const_reference     operator[](size_type index) const   { return (_internal[index]); }
            inline reference           front()                             { return _internal.front(); }
            inline const_reference     front() const                       { return _internal.front(); }
            inline reference           back()                              { return _internal.back(); }
            inline const_reference     back() const                        { return _internal.back(); }
            inline iterator            begin()                             { return _internal.begin(); }
            inline const_iterator      begin() const                       { return _internal.begin(); }
            inline iterator            end()                               { return _internal.end(); }
            inline const_iterator      end() const                         { return _internal.end(); }
            inline const_iterator      cbegin() const                      { return _internal.cbegin(); }
            inline const_iterator      cend() const                        { return _internal.cend(); }
            inline bool operator!=(customVector const& rhs) const { return _internal != rhs._internal; }
            inline bool operator==(customVector const& rhs) const { return _internal == rhs._internal; }
            inline bool operator>(customVector const& rhs) const { return (_internal > rhs._internal); }
            inline bool operator<(customVector const& rhs) const { return (_internal < rhs._internal); }
            inline void push_back(value_type const& value) { _internal.push_back(value); }
            inline void push_back(value_type&& value) { _internal.push_back(std::move(value)); }
            template<typename... Args> inline void emplace_back(Args&&... args) { _internal.emplace_back(std::forward<Args>(args)...); }
            inline void pop_back() { _internal.pop_back(); }
            inline void reserve(size_type capacityUpperBound) { _internal.reserve(capacityUpperBound); }
            inline void resize (size_type n) { _internal.resize(n); }
            inline void resize (size_type n, const value_type& val) { _internal.resize(n, val); }
            inline T* data() { return _internal.data(); }
            inline T* data() const { return _internal.data(); }
            inline void clear() { _internal.clear(); }
            inline iterator erase(const_iterator iter) { return _internal.erase(iter); }
            inline iterator erase(const_iterator first, const_iterator last) { return _internal.erase(first, last); }
            iterator insert(const_iterator position, const T &x) { return _internal.insert(position, x); }
            iterator insert(const_iterator position, T &&x) { return _internal.insert(position, x); }
            template <typename Iterator>
            iterator insert(const_iterator p, Iterator first, Iterator last) { return _internal.insert(p, first, last); }
    };
    #endif
\$\endgroup\$
  • 2
    \$\begingroup\$ Including that many headers will increase compilation time of each translation unit that included it by a lot. It would be great to cut down their numbers. Also, more vertical spacing would be great as well. \$\endgroup\$ – Incomputable May 27 '17 at 10:21
  • 1
    \$\begingroup\$ Also your internal vector is not really internal, since it can be messed with. Personally I don't see what value the code adds to boost's vector. \$\endgroup\$ – Incomputable May 27 '17 at 10:39
  • \$\begingroup\$ Why don't you just use template<class T, class A = std::allocator<T>> using customVector = boost::container::vector<T, A>;? \$\endgroup\$ – Deduplicator May 27 '17 at 11:13
  • \$\begingroup\$ hi, please leave the headers part, i isolated the code from a large project. i cannot use alias, as we do some memory trick in between , actually there are 2 fields missing, one pointer of boost::container::vector and a reference of same type, we kind of detach the internal vector from wrapper in the destructor and reuse it in the constructor, this way we kind of save on memory calls \$\endgroup\$ – Ravikumar Tulugu May 27 '17 at 12:06
  • \$\begingroup\$ I changed the code to reflect the actual scheme of things, please ignore how *internal gets populated, i need to use std::forward on the internal to forward the same arguments being passed to the wrapper \$\endgroup\$ – Ravikumar Tulugu May 27 '17 at 12:10
2
\$\begingroup\$

Well it's broken here:

        boost::container::vector<T, Allocator> *internal = nullptr;
        boost::container::vector<T, Allocator> &_internal = *internal;

De-referencing a nullptr is undefined behavior.

Why are you using boost::container? This is really for use with compilers that have not been updated and use old versions of the standard library. If you have a modern compiler with a modern version of the standard library boost::container becomes irrelevant.

I know you are using a modern compiler because you have the following code in your class:

        using value_type        = typename boost::container::vector<T>::value_type;
        using reference         = typename boost::container::vector<T>::reference;

So it seems redundant to be using the boost::container library.

I don't see any assignment to: internal?

actually there are 2 fields missing, one pointer of boost::container::vector and a reference of same type

What two fields?
No much point in reviewing the code if its not real.

we kind of detach the internal vector from wrapper in the destructor and reuse it in the constructor, this way we kind of save on memory calls

Seems like you are re-inventing the wheel. That's what move semantics is all about.

As it stands I can't see this as real code. It requires some initialization method that is not provided.

Alos it is just a PIMPL around a container class. What exactly do you want to get reviewed?

  • The inclusion of way to many header files that are not relavant to this code.
  • The inclusion of header files from another language (why do you include C files into C++ code).
  • The terrible layout (of half the code, the first half is OK) that makes the code hard to read.
    • But none of these methods do anything. They simply call the same method on the underlying PIMPL(ed) object.

The one improvement you can do is during construction. By retrieving the size of the input container you can reserve enough space for the new elements.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.