Fix some warnings, improve example lifecycle management (VowpalWabbit…

…#2435)

* - Move example cleanup to be part of the class
- Fix double free issue in object_pool
- Fix memset of non-trivially copyable types in object_pool
- Fix parser deletion logic around example pool

* Return all objects to pool in unit test

* Deprecated attribute must come after 'class'

* Add move constructors to example and example_predict, address other comments

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 $<$<CONFIG:DEBUG>:${linux_debug_config}> $<$<CONFIG:RELEASE>:${linux_release_config}> $<$<CONFIG:RelWithDebInfo>:${linux_release_config}>)
+set(linux_flags -Wno-cast-function-type -fvisibility=hidden $<$<CONFIG:DEBUG>:${linux_debug_config}> $<$<CONFIG:RELEASE>:${linux_release_config}> $<$<CONFIG:RelWithDebInfo>:${linux_release_config}>)
 
 if(LTO)
 	if(NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")

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);
 }

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

vowpalwabbit/example.cc

@@ -4,8 +4,120 @@
 #include <cstdint>
 #include <algorithm>
 
+#include "example.h"
 #include "gd.h"
 
+example::example()
+{
+  memset(&l, 0, sizeof(polylabel));
+  memset(&pred, 0, sizeof(polyprediction));
+  tag = v_init<char>();
+}
+
+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<example*>& examples)

vowpalwabbit/example.h

@@ -22,6 +22,8 @@
 #include "ccb_label.h"
 #include "slates_label.h"
 #include "decision_scores.h"
+#include <vector>
+#include <iostream>
 
 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<char> 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<char> 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

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<namespace_index>();
   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<namespace_index>();
+  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();