diff --git a/CMakeLists.txt b/CMakeLists.txt index f266a6dcc98..450613090a0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -104,7 +104,7 @@ if((NOT PROFILE) AND (NOT GCOV)) endif() #Use default visiblity on UNIX otherwise a lot of the C++ symbols end up for exported and interpose'able -set(linux_flags -fvisibility=hidden $<$:${linux_debug_config}> $<$:${linux_release_config}> $<$:${linux_release_config}>) +set(linux_flags -Wno-cast-function-type -fvisibility=hidden $<$:${linux_debug_config}> $<$:${linux_release_config}> $<$:${linux_release_config}>) if(LTO) if(NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang") diff --git a/test/unit_test/object_pool_test.cc b/test/unit_test/object_pool_test.cc index 147b8762712..cecabceaa54 100644 --- a/test/unit_test/object_pool_test.cc +++ b/test/unit_test/object_pool_test.cc @@ -64,4 +64,6 @@ BOOST_AUTO_TEST_CASE(object_pool_test) obj other_obj; BOOST_CHECK_EQUAL(pool.is_from_pool(o2), true); BOOST_CHECK_EQUAL(pool.is_from_pool(&other_obj), false); + + pool.return_object(o2); } diff --git a/vowpalwabbit/CMakeLists.txt b/vowpalwabbit/CMakeLists.txt index fc957627d07..6c15e76300b 100644 --- a/vowpalwabbit/CMakeLists.txt +++ b/vowpalwabbit/CMakeLists.txt @@ -201,6 +201,7 @@ set(vw_all_sources decision_scores.cc distributionally_robust.cc ect.cc + example_predict.cc example.cc explore_eval.cc ftrl.cc diff --git a/vowpalwabbit/example.cc b/vowpalwabbit/example.cc index a66e8783ada..4d6605d449e 100644 --- a/vowpalwabbit/example.cc +++ b/vowpalwabbit/example.cc @@ -4,8 +4,120 @@ #include #include +#include "example.h" #include "gd.h" +example::example() +{ + memset(&l, 0, sizeof(polylabel)); + memset(&pred, 0, sizeof(polyprediction)); + tag = v_init(); +} + +example::~example() +{ + tag.delete_v(); + if (passthrough) + { + delete passthrough; + passthrough = nullptr; + } +} + +example::example(example&& other) noexcept + : example_predict(std::move(other)) + , l(other.l) + , pred(other.pred) + , weight(other.weight) + , tag(std::move(other.tag)) + , example_counter(other.example_counter) + , num_features(other.num_features) + , partial_prediction(other.partial_prediction) + , updated_prediction(other.updated_prediction) + , loss(other.loss) + , total_sum_feat_sq(other.total_sum_feat_sq) + , confidence(other.confidence) + , passthrough(other.passthrough) + , test_only(other.test_only) + , end_pass(other.end_pass) + , sorted(other.sorted) + , in_use(other.in_use) +{ + other.weight = 1.f; + auto& other_tag = other.tag; + other_tag._begin = nullptr; + other_tag._end = nullptr; + other_tag.end_array = nullptr; + other.example_counter = 0; + other.num_features = 0; + other.partial_prediction = 0.f; + other.updated_prediction = 0.f; + other.loss = 0.f; + other.total_sum_feat_sq = 0.f; + other.confidence = 0.f; + other.passthrough = nullptr; + other.test_only = false; + other.end_pass = false; + other.sorted = false; + other.in_use = false; +} + +example& example::operator=(example&& other) noexcept +{ + example_predict::operator=(std::move(other)); + l = other.l; + pred = other.pred; + weight = other.weight; + tag = std::move(other.tag); + example_counter = other.example_counter; + num_features = other.num_features; + partial_prediction = other.partial_prediction; + updated_prediction = other.updated_prediction; + loss = other.loss; + total_sum_feat_sq = other.total_sum_feat_sq; + confidence = other.confidence; + passthrough = other.passthrough; + test_only = other.test_only; + end_pass = other.end_pass; + sorted = other.sorted; + in_use = other.in_use; + + other.weight = 1.f; + + // We need to null out all the v_arrays to prevent double freeing during moves + auto& other_tag = other.tag; + other_tag._begin = nullptr; + other_tag._end = nullptr; + other_tag.end_array = nullptr; + + other.example_counter = 0; + other.num_features = 0; + other.partial_prediction = 0.f; + other.updated_prediction = 0.f; + other.loss = 0.f; + other.total_sum_feat_sq = 0.f; + other.confidence = 0.f; + other.passthrough = nullptr; + other.test_only = false; + other.end_pass = false; + other.sorted = false; + other.in_use = false; + return *this; +} + +void example::delete_unions(void (*delete_label)(void*), void (*delete_prediction)(void*)) +{ + if (delete_label) + { + delete_label(&l); + } + + if (delete_prediction) + { + delete_prediction(&pred); + } +} + float collision_cleanup(features& fs) { uint64_t last_index = (uint64_t)-1; @@ -214,20 +326,7 @@ example* alloc_examples(size_t, size_t count = 1) void dealloc_example(void (*delete_label)(void*), example& ec, void (*delete_prediction)(void*)) { - if (delete_label) - delete_label(&ec.l); - - if (delete_prediction) - delete_prediction(&ec.pred); - - ec.tag.delete_v(); - - if (ec.passthrough) - { - delete ec.passthrough; - } - - ec.indices.delete_v(); + ec.delete_unions(delete_label, delete_prediction); ec.~example(); } @@ -236,8 +335,7 @@ void clean_example(vw&, example&, bool rewind); void finish_example(vw& all, multi_ex& ec_seq) { - for (example* ecc : ec_seq) - VW::finish_example(all, *ecc); + for (example* ecc : ec_seq) VW::finish_example(all, *ecc); } void return_multiple_example(vw& all, v_array& examples) diff --git a/vowpalwabbit/example.h b/vowpalwabbit/example.h index 69ccad05bf9..21ec58a4f77 100644 --- a/vowpalwabbit/example.h +++ b/vowpalwabbit/example.h @@ -22,6 +22,8 @@ #include "ccb_label.h" #include "slates_label.h" #include "decision_scores.h" +#include +#include typedef union { @@ -56,31 +58,46 @@ typedef union IGNORE_DEPRECATED_USAGE_START struct example : public example_predict // core example datatype. { + example(); + ~example(); + + example(const example&) = delete; + example& operator=(const example&) = delete; + example(example&& other) noexcept; + example& operator=(example&& other) noexcept; + + /// Example contains unions for label and prediction. These do not get cleaned + /// up by the constructor because the type is not known at that time. To + /// ensure correct cleanup delete_unions must be explicitly called. + void delete_unions(void (*delete_label)(void*), void (*delete_prediction)(void*)); + // input fields polylabel l; // output prediction polyprediction pred; - float weight; // a relative importance weight for the example, default = 1 - v_array tag; // An identifier for the example. - size_t example_counter; + float weight = 1.f; // a relative importance weight for the example, default = 1 + v_array tag; // An identifier for the example. + size_t example_counter = 0; // helpers - size_t num_features; // precomputed, cause it's fast&easy. - float partial_prediction; // shared data for prediction. - float updated_prediction; // estimated post-update prediction. - float loss; - float total_sum_feat_sq; // precomputed, cause it's kind of fast & easy. - float confidence; - features* - passthrough; // if a higher-up reduction wants access to internal state of lower-down reductions, they go here - - bool test_only; - bool end_pass; // special example indicating end of pass. - bool sorted; // Are the features sorted or not? - - VW_DEPRECATED("in_use has been removed, examples taken from the pool are assumed to be in use if there is a reference to them. Standalone examples are by definition always in use.") + size_t num_features = 0; // precomputed, cause it's fast&easy. + float partial_prediction = 0.f; // shared data for prediction. + float updated_prediction = 0.f; // estimated post-update prediction. + float loss = 0.f; + float total_sum_feat_sq = 0.f; // precomputed, cause it's kind of fast & easy. + float confidence = 0.f; + features* passthrough = + nullptr; // if a higher-up reduction wants access to internal state of lower-down reductions, they go here + + bool test_only = false; + bool end_pass = false; // special example indicating end of pass. + bool sorted = false; // Are the features sorted or not? + + VW_DEPRECATED( + "in_use has been removed, examples taken from the pool are assumed to be in use if there is a reference to them. " + "Standalone examples are by definition always in use.") bool in_use = true; }; IGNORE_DEPRECATED_USAGE_END diff --git a/vowpalwabbit/example_predict.cc b/vowpalwabbit/example_predict.cc index 0031a46b4e0..5a0f1d0149a 100644 --- a/vowpalwabbit/example_predict.cc +++ b/vowpalwabbit/example_predict.cc @@ -4,18 +4,76 @@ #include "example_predict.h" -safe_example_predict::safe_example_predict() +example_predict::iterator::iterator(features* feature_space, namespace_index* index) + : _feature_space(feature_space), _index(index) +{ +} + +features& example_predict::iterator::operator*() { return _feature_space[*_index]; } + +example_predict::iterator& example_predict::iterator::operator++() +{ + _index++; + return *this; +} + +namespace_index example_predict::iterator::index() { return *_index; } + +bool example_predict::iterator::operator==(const iterator& rhs) { return _index == rhs._index; } +bool example_predict::iterator::operator!=(const iterator& rhs) { return _index != rhs._index; } + +example_predict::example_predict() { indices = v_init(); ft_offset = 0; - // feature_space is initialized through constructors + interactions = nullptr; } -safe_example_predict::~safe_example_predict() +example_predict::~example_predict() { indices.delete_v(); } + +example_predict::example_predict(example_predict&& other) noexcept + : indices(std::move(other.indices)) + , feature_space(std::move(other.feature_space)) + , ft_offset(other.ft_offset) + , interactions(other.interactions) { - indices.delete_v(); + // We need to null out all the v_arrays to prevent double freeing during moves + auto& v = other.indices; + v._begin = nullptr; + v._end = nullptr; + v.end_array = nullptr; + other.ft_offset = 0; + other.interactions = nullptr; } +example_predict& example_predict::operator=(example_predict&& other) noexcept +{ + indices = std::move(other.indices); + feature_space = std::move(other.feature_space); + interactions = other.interactions; + // We need to null out all the v_arrays to prevent double freeing during moves + + auto& v = other.indices; + v._begin = nullptr; + v._end = nullptr; + v.end_array = nullptr; + other.ft_offset = 0; + other.interactions = nullptr; + return *this; +} + +example_predict::iterator example_predict::begin() { return {feature_space.data(), indices.begin()}; } +example_predict::iterator example_predict::end() { return {feature_space.data(), indices.end()}; } + +safe_example_predict::safe_example_predict() +{ + indices = v_init(); + ft_offset = 0; + // feature_space is initialized through constructors +} + +safe_example_predict::~safe_example_predict() { indices.delete_v(); } + void safe_example_predict::clear() { for (auto ns : indices) feature_space[ns].clear(); diff --git a/vowpalwabbit/example_predict.h b/vowpalwabbit/example_predict.h index 5bfb414bd97..06464ec490e 100644 --- a/vowpalwabbit/example_predict.h +++ b/vowpalwabbit/example_predict.h @@ -8,6 +8,7 @@ typedef unsigned char namespace_index; #include "v_array.h" #include "feature_group.h" #include "constant.h" +#include "future_compat.h" #include #include @@ -16,39 +17,42 @@ struct example_predict class iterator { features* _feature_space; - namespace_index* _index; + v_array::iterator _index; public: - iterator(features* feature_space, namespace_index* index) : _feature_space(feature_space), _index(index) {} - - features& operator*() { return _feature_space[*_index]; } - - iterator& operator++() - { - _index++; - return *this; - } + iterator(features* feature_space, namespace_index* index); + features& operator*(); + iterator& operator++(); + namespace_index index(); + bool operator==(const iterator& rhs); + bool operator!=(const iterator& rhs); + }; - namespace_index index() { return *_index; } + example_predict(); + ~example_predict(); + example_predict(const example_predict&) = delete; + example_predict& operator=(const example_predict&) = delete; + example_predict(example_predict&& other) noexcept; + example_predict& operator=(example_predict&& other) noexcept; - bool operator==(const iterator& rhs) { return _index == rhs._index; } - bool operator!=(const iterator& rhs) { return _index != rhs._index; } - }; + /// If indices is modified this iterator is invalidated. + iterator begin(); + /// If indices is modified this iterator is invalidated. + iterator end(); v_array indices; std::array feature_space; // Groups of feature values. uint64_t ft_offset; // An offset for all feature values. - // Interactions are specified by this vector of vectors of unsigned characters, where each vector is an interaction and each char is a - // namespace. + // Interactions are specified by this vector of vectors of unsigned characters, where each vector is an interaction + // and each char is a namespace. std::vector>* interactions; - - iterator begin() { return iterator(feature_space.data(), indices.begin()); } - iterator end() { return iterator(feature_space.data(), indices.end()); } }; // make sure we have an exception safe version of example_predict -class safe_example_predict : public example_predict + +class VW_DEPRECATED("example_predict is now RAII based. That class can be used instead.") safe_example_predict + : public example_predict { public: safe_example_predict(); diff --git a/vowpalwabbit/interactions.cc b/vowpalwabbit/interactions.cc index 96248406ced..bda0047d460 100644 --- a/vowpalwabbit/interactions.cc +++ b/vowpalwabbit/interactions.cc @@ -294,7 +294,7 @@ void eval_count_of_generated_ft(vw& all, example& ec, size_t& new_features_cnt, ++order_of_inter; // namespace is same for whole block - features& fs = ec.feature_space[(const int)*ns]; + features& fs = ec.feature_space[static_cast(*ns)]; // count number of features with value != 1.; size_t cnt_ft_value_non_1 = 0; diff --git a/vowpalwabbit/object_pool.h b/vowpalwabbit/object_pool.h index 8fe57e48fdc..3183fd049c0 100644 --- a/vowpalwabbit/object_pool.h +++ b/vowpalwabbit/object_pool.h @@ -42,6 +42,7 @@ struct no_lock_object_pool ~no_lock_object_pool() { + assert(m_pool.size() == size()); while (!m_pool.empty()) { auto front = m_pool.front(); @@ -112,8 +113,6 @@ struct no_lock_object_pool for (size_t i = 0; i < size; i++) { - memset(&chunk[i], 0, sizeof(T)); - new (&chunk[i]) T{}; m_pool.push(m_initializer(&chunk[i])); } } diff --git a/vowpalwabbit/parser.cc b/vowpalwabbit/parser.cc index 5c2c122a9ee..c18203d558d 100644 --- a/vowpalwabbit/parser.cc +++ b/vowpalwabbit/parser.cc @@ -1015,39 +1015,23 @@ namespace VW void start_parser(vw& all) { all.parse_thread = std::thread(main_parse_loop, &all); } } // namespace VW -// a copy of dealloc_example except that this does not call the example destructor -// Work to remove this is currently in progress -void cleanup_example(void(*delete_label)(void*), example& ec, void(*delete_prediction)(void*)) -{ - if (delete_label) - delete_label(&ec.l); - - if (delete_prediction) - delete_prediction(&ec.pred); - - ec.tag.delete_v(); - - if (ec.passthrough) - { - delete ec.passthrough; - } - - ec.indices.delete_v(); -} - void free_parser(vw& all) { + std::vector drain_pool; + drain_pool.reserve(all.p->example_pool.size()); while (!all.p->example_pool.empty()) { example* temp = all.p->example_pool.get_object(); - cleanup_example(all.p->lp.delete_label, *temp, all.delete_prediction); + temp->delete_unions(all.p->lp.delete_label, all.delete_prediction); + drain_pool.push_back(temp); } - - while (all.p->ready_parsed_examples.size() != 0) + for(auto* example_ptr : drain_pool) { - example* temp = all.p->ready_parsed_examples.pop(); - cleanup_example(all.p->lp.delete_label, *temp, all.delete_prediction); + all.p->example_pool.return_object(example_ptr); } + + // There should be no examples in flight at this point. + assert(all.p->ready_parsed_examples.size() == 0); } namespace VW diff --git a/vowpalwabbit/stagewise_poly.cc b/vowpalwabbit/stagewise_poly.cc index e8883e7843a..36a801f67d5 100644 --- a/vowpalwabbit/stagewise_poly.cc +++ b/vowpalwabbit/stagewise_poly.cc @@ -76,9 +76,11 @@ struct stagewise_poly #endif // DEBUG //synth_ec.feature_space[tree_atomics].delete_v(); - synth_ec.indices.delete_v(); free(sd); free(depthsbits); + + // Intentionally do not clear the unions here. + synth_ec.delete_unions(nullptr, nullptr); } }; diff --git a/vowpalwabbit/v_array.h b/vowpalwabbit/v_array.h index af8553eb5d7..28633a48ce5 100644 --- a/vowpalwabbit/v_array.h +++ b/vowpalwabbit/v_array.h @@ -36,6 +36,8 @@ struct v_array T* end_array; size_t erase_count; + using iterator = T*; + // enable C++ 11 for loops inline T*& begin() { return _begin; } inline T*& end() { return _end; } diff --git a/vowpalwabbit/vw_core.vcxproj b/vowpalwabbit/vw_core.vcxproj index 8c533303ddc..75553976544 100644 --- a/vowpalwabbit/vw_core.vcxproj +++ b/vowpalwabbit/vw_core.vcxproj @@ -272,6 +272,7 @@ +