пятница, 6 февраля 2015 г.

[prog.flame.c++] А потом говорят, что C++ ущербный язык...

По следам недавней темы заглянул в кишки проекта disruptor-cpp. На первый взгляд, это калька Disruptor-а с Java на C++. Вроде как и сделанная нормально. Но, в то же время, хороший пример того, что не нужно делать в современном C++. И об этом чуть подробнее под катом.

Так уж получилось, что первым файлом, куда я заглянул, оказался файл ring_buffer.h. Для простоты просто приведу код шаблонного класса RingBuffer целиком, благо его немного:

// Ring based store of reusable entries containing the data representing an
// event beign exchanged between publisher and {@link EventProcessor}s.
//
// @param <T> implementation storing the data for sharing during exchange
// or parallel coordination of an event.
template<typename T>
class RingBuffer : public Sequencer {
 public:
    // Construct a RingBuffer with the full option set.
    //
    // @param event_factory to instance new entries for filling the RingBuffer.
    // @param buffer_size of the RingBuffer, must be a power of 2.
    // @param claim_strategy_option threading strategy for publishers claiming
    // entries in the ring.
    // @param wait_strategy_option waiting strategy employed by
    // processors_to_track waiting in entries becoming available.
    RingBuffer(EventFactoryInterface<T>* event_factory,
               int buffer_size,
               ClaimStrategyOption claim_strategy_option,
               WaitStrategyOption wait_strategy_option) :
            Sequencer(buffer_size,
                      claim_strategy_option,
                      wait_strategy_option),
            buffer_size_(buffer_size),
            mask_(buffer_size - 1),
            events_(event_factory->NewInstance(buffer_size)) {
    }

    ~RingBuffer() {
        delete[] events_;
    }

    // Get the event for a given sequence in the RingBuffer.
    //
    // @param sequence for the event
    // @return event pointer at the specified sequence position.
    T* Get(const int64_t& sequence) {
        return &events_[sequence & mask_];
    }

 private:
    // Members
    int buffer_size_;
    int mask_;
    T* events_;

    DISALLOW_COPY_AND_ASSIGN(RingBuffer);
};

Это, на минуточку, код для C++11. Но полное ощущение, что человек осваивал C++ еще в те времена, когда STL-я вообще не было.

Главная моя претензция -- это голый указатель events_. В наше время оправданий для работы с голыми указателями в C++ очень мало. В данном случае их вообще нет.

Во-первых, от голого указателя можно было бы избавиться посредством std::vector. Метод EventFactoryInterface<T>::NewInstance() возвращал бы std::vector<T> по значению. За счет move semantics возвращенное из NetInstance() значение перемещалось бы в event_ без лишних накладных расходов. И все! Никаких new внутри фабрики. Никаких деструкторов в самом RingBuffer-е.

Кстати о том, что вместо new T[N] в современном C++ нужно использовать std::vector<T>(N) Бьёрн Страуструп писал еще в третьем издании "Язык программирования C++" в самом-самом начале 2000-х, т.е. почти 15 лет тому назад.

Во-вторых, если std::vector по каким-то религиозным причинам не проходит, то почему нельзя было задействовать std::unique_ptr? В С++11 std::unique_ptr, в отличии от std::auto_ptr из C++98/03, совершенно спокойно поддерживает T[].

Далее. Почему аргумент event_factory передается в конструктор по указателю? Чтобы nullptr подсунуть проще было? Или у автора стойкая нелюбовь к ссылкам?

Аналогичный вопрос можно задать и по поводу возвращаемого значения метода Get. Какой смысл возвращать указатель и почему нельзя было вернуть ссылку?

Однако, самый главный пакет претензий вызывает то, что связано с заданием размера буфера.

Начнем с того, что в C++, в отличии от Java, есть беззнаковые целые типы. И размерности векторов, а так же индексы элементов в них принято задавать посредством безннаковых целых типов. Причем желательно не просто unsigned int, а std::size_t, дабы не иметь проблем при переходе между 16-/32-/64-битовыми платформами.

Далее, многие ли обратили внимание, насколько критично требование о том, чтобы значение buffer_size было степенью двойки? Кстати, если я правильно помню, единица -- это тоже степень двойки, нулевая :)

Но нигде в сигнатуре конструктора RingBuffer нет даже попытки придать этому требованию более жесткий и формальный вид. Хотя в C++, в отличии от Java, сделать это вообще не проблема. Основы этой техники идут в Виртовский Pascal, где можно было объявлять подтипы существющих типов. Этой же технике было посвящено несколько слайдов в презентации про использование Haskell-я в Standard Chartered Bank (слайды с 16-го по 20, как минимум).

Всего-то и нужно было -- определить махонький вспомогательный класс:

struct SizeAsPowerOfTwo
{
   const std::size_t v;

   SizeAsPowerOfTwo( std::size_t a )
      :  v( EnsureValidity( a ) )
   {}

   inline static std::size_t
   EnsureValidity( std::size_t a )
   {
      if( a <= 1 || (0 != (a & (a-1))) )
         throw std::invalid_argument( std::to_string(a) +
               ": is not a valid buffer size" );
      return a;
   }
};

И использовать тип size_as_power_of_two в качестве типа параметра buffer_size в конструкторе RingBuffer-а.

Ну и еще одно отличие C++ от Java -- атрибуты класса можно делать константными. В данном случае атрибуты buffer_size_ и mask_ следовало бы сделать таковыми, т.к. класс RingBuffer не предполагает их изменения в принципе.

С константностью связан еще один момент вокруг метода Get. По идее, его тоже можно бы объявить константным, т.к. вызов Get не должен менять содержимое RingBuffer-а. Однако, если Get будет константным, а event_ будет представлен как std::vector, то event_ нужно будет помечать как mutable-атрибут класса, иначе из Get-а нельзя будет вернуть неконстантную ссылку на T. Кого-то может смущать наличие mutable-атрибута в классе. Кроме того, может быть RingBuffer-у не помешало бы вообще иметь два метода Get -- константный и неконстантный. Здесь нужно отталкиваться от сценариев использования.

Ну и еще, если говорить про метод Get, не факт, что sequence туда нужно передавать по ссылке. Метод inline-овый, sequence используется один раз внутри арифметического выражения. Вполне возможно, что передача по значению была бы даже эффективнее. Так что я бы сначала сделал передачу по значению, а уже если профайлер укажет, что здесь есть просадка, вернулся бы к передаче по ссылке.

Кроме того, если уж мы перешли от int-а к std::size_t, то и тип аргумента sequence для Get нужно преобразовать к uint64_t, дабы не было смешения знаковых и беззнаковых значений в одном выражении.

Итого, если собрать все вместе, то следовало бы написать что-то вроде (предупреждаю, это не компилировалось):

Upd. Атрибут buffer_size_ вообще был убран за ненадобностью.

// Ring based store of reusable entries containing the data representing an
// event beign exchanged between publisher and {@link EventProcessor}s.
//
// @param <T> implementation storing the data for sharing during exchange
// or parallel coordination of an event.
template<typename T>
class RingBuffer : public Sequencer {
 public:
    // Construct a RingBuffer with the full option set.
    //
    // @param event_factory to instance new entries for filling the RingBuffer.
    // @param buffer_size of the RingBuffer, must be a power of 2.
    // @param claim_strategy_option threading strategy for publishers claiming
    // entries in the ring.
    // @param wait_strategy_option waiting strategy employed by
    // processors_to_track waiting in entries becoming available.
    RingBuffer(EventFactoryInterface<T> &  event_factory,
               SizeAsPowerOfTwo buffer_size,
               ClaimStrategyOption claim_strategy_option,
               WaitStrategyOption wait_strategy_option) :
            Sequencer(buffer_size,
                      claim_strategy_option,
                      wait_strategy_option),
            mask_(buffer_size.v - 1),
            events_(event_factory.NewInstance(buffer_size.v)) {
    }

    // Get the event for a given sequence in the RingBuffer.
    //
    // @param sequence for the event
    // @return event pointer at the specified sequence position.
    const T& Get(uint64_t sequence) const {
        return events_[sequence & mask_];
    }
    // Get the event for a given sequence in the RingBuffer.
    //
    // @param sequence for the event
    // @return event pointer at the specified sequence position.
    T& Get(uint64_t sequence) {
        return events_[sequence & mask_];
    }

 private:
    // Members
    const std::size_t mask_;
    std::vector<T> events_;

    DISALLOW_COPY_AND_ASSIGN(RingBuffer);
};

Ну и здесь еще имело бы смысл посмотреть на то, а следует ли RingBuffer защищать от копирования и присваивания. Хотя бы потому, что от этого уже защищен его базовый класс, Sequencer. Так что я бы еще убрал бы и использование макроса DISALLOW_COPY_AND_ASSIGN.


Зачем все это было написано? Да просто накатывает временами. В C++ уже лет двадцать как есть нормальные средства, чтобы писать с минимальными шансами отстрелить себе что-нибудь. И чем старше C++, тем больше у него этих средств и возможностей. Но как посмотришь на какой-нибудь недавно написанный код, так блин, реальный привет из конца 80-х. Ну нельзя же так, в конце-то концов.

PS. Трюк с определением того, что число является степенью двойки был найден по ссылкам из старого поста.

Комментариев нет: