VowpalWabbit / vowpal_wabbit

Vowpal Wabbit is a machine learning system which pushes the frontier of machine learning with techniques such as online, hashing, allreduce, reductions, learning2search, active, and interactive learning.
https://vowpalwabbit.org
Other
8.49k stars 1.93k forks source link

v_array should only contain TriviallyCopyable types #2205

Closed jackgerrits closed 3 years ago

jackgerrits commented 4 years ago

v_array uses std::realloc to obtain better performance than std::vector in many places. However, as realloc can copy the piece of memory when it must be expanded this is undefined behavior unless the contained types are TriviallyCopyable.

The following code snippet will enable this to be compile time guaranteed. However, after changing to this it is clear we are violating this in quite a few places in the codebase and will require some work to fix.

// If you get an error message saying that x uses undefined struct 'v_array<...,void>' that means the type
// is not trivially copyable and cannot be used with v_array.
template <typename T, typename Enable = void>
struct v_array;

// v_array makes use of realloc for efficiency. However, it is only safe to use trivially copyable types,
// as std::realloc may do a memcpy if a new piece of memory must be allocated.
template <class T>
struct v_array<T, typename std::enable_if<std::is_trivially_copyable<T>::value>::type>
{
//...
jackgerrits commented 4 years ago

This is now being highlighted by warnings in GCC.

In file included from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/action_score.h:9,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/conditional_contextual_bandit.h:12,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/ccb_label.cc:5:
/home/jack/w/repos/vowpal_wabbit/vowpalwabbit/v_array.h: In instantiation of ‘void v_array<T>::resize(size_t) [with T = boost::basic_string_view<char, std::char_traits<char> >; size_t = long unsigned int]’:
/home/jack/w/repos/vowpal_wabbit/vowpalwabbit/v_array.h:92:7:   required from ‘void v_array<T>::clear() [with T = boost::basic_string_view<char, std::char_traits<char> >]’
/home/jack/w/repos/vowpal_wabbit/vowpalwabbit/ccb_label.cc:254:22:   required from here
/home/jack/w/repos/vowpal_wabbit/vowpalwabbit/v_array.h:82:15: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘class boost::basic_string_view<char, std::char_traits<char> >’; use assignment or value-initialization instead [-Wclass-memaccess]
   82 |         memset(_begin + old_len, 0, (length - old_len) * sizeof(T));
      |         ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/vw_string_view.h:13,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/parse_primitives.h:15,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/label_parser.h:8,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/no_label.h:5,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/example.h:11,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/reductions.h:6,
                 from /home/jack/w/repos/vowpal_wabbit/vowpalwabbit/ccb_label.cc:6:
/usr/include/boost/utility/string_view.hpp:55:11: note: ‘class boost::basic_string_view<char, std::char_traits<char> >’ declared here
   55 |     class basic_string_view {
      |           ^~~~~~~~~~~~~~~~~
jackgerrits commented 4 years ago

Partially addressed in #2484. Only search left to fix.

Once that is done the enable_if should be able to be easily added to guard future usage.

jackgerrits commented 4 years ago

This can't be completed until some APIs are removed. Deprecation began in #2498.