You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by jm...@apache.org on 2023/01/28 04:44:56 UTC

[datasketches-cpp] 01/01: use allocator properly in varopt union

This is an automated email from the ASF dual-hosted git repository.

jmalkin pushed a commit to branch var_opt_union_allocator
in repository https://gitbox.apache.org/repos/asf/datasketches-cpp.git

commit 9ba368fd7090e77d0b853fa2aa70f2ca7dfcaea8
Author: Jon <jm...@apache.org>
AuthorDate: Fri Jan 27 20:44:38 2023 -0800

    use allocator properly in varopt union
---
 sampling/include/var_opt_sketch_impl.hpp | 14 +++++++-------
 sampling/include/var_opt_union.hpp       |  6 +++++-
 sampling/include/var_opt_union_impl.hpp  | 32 ++++++++++++++++++--------------
 3 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/sampling/include/var_opt_sketch_impl.hpp b/sampling/include/var_opt_sketch_impl.hpp
index 2c8db2b..ecbee26 100644
--- a/sampling/include/var_opt_sketch_impl.hpp
+++ b/sampling/include/var_opt_sketch_impl.hpp
@@ -189,16 +189,16 @@ var_opt_sketch<T, A>::~var_opt_sketch() {
       // destroy everything
       const size_t num_to_destroy = std::min(k_ + 1, curr_items_alloc_);
       for (size_t i = 0; i < num_to_destroy; ++i) {
-        allocator_.destroy(data_ + i);
+        data_[i].~T();
       }
     } else {
       // skip gap or anything unused at the end
       for (size_t i = 0; i < h_; ++i) {
-        allocator_.destroy(data_+ i);
+        data_[i].~T();
       }
     
       for (size_t i = h_ + 1; i < h_ + r_ + 1; ++i) {
-        allocator_.destroy(data_ + i);
+        data_[i].~T();
       }
     }
     allocator_.deallocate(data_, curr_items_alloc_);
@@ -658,14 +658,14 @@ void var_opt_sketch<T, A>::reset() {
     // destroy everything
     const size_t num_to_destroy = std::min(k_ + 1, prev_alloc);
     for (size_t i = 0; i < num_to_destroy; ++i) 
-      allocator_.destroy(data_ + i);
+      data_[i].~T();
   } else {
     // skip gap or anything unused at the end
     for (size_t i = 0; i < h_; ++i)
-      allocator_.destroy(data_+ i);
+      data_[i].~T();
     
     for (size_t i = h_ + 1; i < h_ + r_ + 1; ++i)
-      allocator_.destroy(data_ + i);
+      data_[i].~T();
   }
 
   if (curr_items_alloc_ < prev_alloc) {
@@ -990,7 +990,7 @@ void var_opt_sketch<T, A>::grow_data_arrays() {
 
     for (uint32_t i = 0; i < prev_size; ++i) {
       new (&tmp_data[i]) T(std::move(data_[i]));
-      allocator_.destroy(data_ + i);
+      data_[i].~T();
       tmp_weights[i] = weights_[i];
     }
 
diff --git a/sampling/include/var_opt_union.hpp b/sampling/include/var_opt_union.hpp
index 1bc0b1b..3d00735 100644
--- a/sampling/include/var_opt_union.hpp
+++ b/sampling/include/var_opt_union.hpp
@@ -153,6 +153,8 @@ public:
 
 private:
   typedef typename std::allocator_traits<A>::template rebind_alloc<var_opt_sketch<T, A>> AllocSketch;
+  typedef typename std::allocator_traits<A>::template rebind_alloc<double> AllocDouble;
+  typedef typename std::allocator_traits<A>::template rebind_alloc<bool> AllocBool;
 
   static const uint8_t PREAMBLE_LONGS_EMPTY = 1;
   static const uint8_t PREAMBLE_LONGS_NON_EMPTY = 4;
@@ -170,10 +172,12 @@ private:
 
   uint32_t max_k_;
 
+  A allocator_;
+
   var_opt_sketch<T, A> gadget_;
 
   var_opt_union(uint64_t n, double outer_tau_numer, uint64_t outer_tau_denom,
-                uint32_t max_k, var_opt_sketch<T, A>&& gadget);
+                uint32_t max_k, var_opt_sketch<T, A>&& gadget, const A& allocator = A());
 
   /*
    IMPORTANT NOTE: the "gadget" in the union object appears to be a varopt sketch,
diff --git a/sampling/include/var_opt_union_impl.hpp b/sampling/include/var_opt_union_impl.hpp
index 1154061..b717f27 100644
--- a/sampling/include/var_opt_union_impl.hpp
+++ b/sampling/include/var_opt_union_impl.hpp
@@ -34,6 +34,7 @@ var_opt_union<T, A>::var_opt_union(uint32_t max_k, const A& allocator) :
   outer_tau_numer_(0.0),
   outer_tau_denom_(0),
   max_k_(max_k),
+  allocator_(allocator),
   gadget_(max_k, var_opt_sketch<T, A>::DEFAULT_RESIZE_FACTOR, true, allocator)
 {}
 
@@ -43,6 +44,7 @@ var_opt_union<T, A>::var_opt_union(const var_opt_union& other) :
   outer_tau_numer_(other.outer_tau_numer_),
   outer_tau_denom_(other.outer_tau_denom_),
   max_k_(other.max_k_),
+  allocator_(other.allocator_),
   gadget_(other.gadget_)
 {}
 
@@ -52,16 +54,18 @@ var_opt_union<T, A>::var_opt_union(var_opt_union&& other) noexcept :
   outer_tau_numer_(other.outer_tau_numer_),
   outer_tau_denom_(other.outer_tau_denom_),
   max_k_(other.max_k_),
+  allocator_(other.allocator_),
   gadget_(std::move(other.gadget_))
 {}
 
 template<typename T, typename A>
 var_opt_union<T, A>::var_opt_union(uint64_t n, double outer_tau_numer, uint64_t outer_tau_denom,
-                                    uint32_t max_k, var_opt_sketch<T, A>&& gadget) :
+                                    uint32_t max_k, var_opt_sketch<T, A>&& gadget, const A& allocator) :
   n_(n),
   outer_tau_numer_(outer_tau_numer),
   outer_tau_denom_(outer_tau_denom),
   max_k_(max_k),
+  allocator_(allocator),
   gadget_(gadget)
 {}
 
@@ -75,6 +79,7 @@ var_opt_union<T, A>& var_opt_union<T, A>::operator=(const var_opt_union& other)
   std::swap(outer_tau_numer_, union_copy.outer_tau_numer_);
   std::swap(outer_tau_denom_, union_copy.outer_tau_denom_);
   std::swap(max_k_, union_copy.max_k_);
+  std::swap(allocator_, other.allocator_);
   std::swap(gadget_, union_copy.gadget_);
   return *this;
 }
@@ -85,6 +90,7 @@ var_opt_union<T, A>& var_opt_union<T, A>::operator=(var_opt_union&& other) {
   std::swap(outer_tau_numer_, other.outer_tau_numer_);
   std::swap(outer_tau_denom_, other.outer_tau_denom_);
   std::swap(max_k_, other.max_k_);
+  std::swap(allocator_, other.allocator_);
   std::swap(gadget_, other.gadget_);
   return *this;
 }
@@ -162,7 +168,7 @@ var_opt_union<T, A> var_opt_union<T, A>::deserialize(std::istream& is, const Ser
   if (!is.good())
     throw std::runtime_error("error reading from std::istream"); 
 
-  return var_opt_union(items_seen, outer_tau_numer, outer_tau_denom, max_k, std::move(gadget));
+  return var_opt_union(items_seen, outer_tau_numer, outer_tau_denom, max_k, std::move(gadget), allocator);
 }
 
 template<typename T, typename A>
@@ -204,7 +210,7 @@ var_opt_union<T, A> var_opt_union<T, A>::deserialize(const void* bytes, size_t s
   const size_t gadget_size = size - (PREAMBLE_LONGS_NON_EMPTY << 3);
   var_opt_sketch<T, A> gadget = var_opt_sketch<T, A>::deserialize(ptr, gadget_size, sd, allocator);
 
-  return var_opt_union(items_seen, outer_tau_numer, outer_tau_denom, max_k, std::move(gadget));
+  return var_opt_union(items_seen, outer_tau_numer, outer_tau_denom, max_k, std::move(gadget), allocator);
 }
 
 template<typename T, typename A>
@@ -508,9 +514,8 @@ void var_opt_union<T, A>::mark_moving_gadget_coercer(var_opt_sketch<T, A>& sk) c
   uint32_t result_r = 0;
   size_t next_r_pos = result_k; // = (result_k+1)-1, to fill R region from back to front
 
-  typedef typename std::allocator_traits<A>::template rebind_alloc<double> AllocDouble;
-  double* wts = AllocDouble().allocate(result_k + 1);
-  T* data     = A().allocate(result_k + 1);
+  double* wts = AllocDouble(allocator_).allocate(result_k + 1);
+  T* data     = A(allocator_).allocate(result_k + 1);
     
   // insert R region items, ignoring weights
   // Currently (May 2017) this next block is unreachable; this coercer is used only in the
@@ -519,7 +524,7 @@ void var_opt_union<T, A>::mark_moving_gadget_coercer(var_opt_sketch<T, A>& sk) c
   // Addedndum (Jan 2020): Cleanup at end of method assumes R count is 0
   const size_t final_idx = gadget_.get_num_samples();
   for (size_t idx = gadget_.h_ + 1; idx <= final_idx; ++idx) {
-    A().construct(&data[next_r_pos], T(gadget_.data_[idx]));
+    new (&data[next_r_pos]) T(gadget_.data_[idx]);
     wts[next_r_pos]  = gadget_.weights_[idx];
     ++result_r;
     --next_r_pos;
@@ -530,13 +535,13 @@ void var_opt_union<T, A>::mark_moving_gadget_coercer(var_opt_sketch<T, A>& sk) c
   // insert H region items
   for (size_t idx = 0; idx < gadget_.h_; ++idx) {
     if (gadget_.marks_[idx]) {
-      A().construct(&data[next_r_pos], T(gadget_.data_[idx]));
+      new (&data[next_r_pos]) T(gadget_.data_[idx]);
       wts[next_r_pos] = -1.0;
       transferred_weight += gadget_.weights_[idx];
       ++result_r;
       --next_r_pos;
     } else {
-      A().construct(&data[result_h], T(gadget_.data_[idx]));
+      new (&data[result_h]) T(gadget_.data_[idx]);
       wts[result_h] = gadget_.weights_[idx];
       ++result_h;
     }
@@ -554,11 +559,10 @@ void var_opt_union<T, A>::mark_moving_gadget_coercer(var_opt_sketch<T, A>& sk) c
   wts[result_h] = -1.0;
 
   // clean up arrays in input sketch, replace with new values
-  typedef typename std::allocator_traits<A>::template rebind_alloc<bool> AllocBool;
-  AllocBool().deallocate(sk.marks_, sk.curr_items_alloc_);
-  AllocDouble().deallocate(sk.weights_, sk.curr_items_alloc_);
-  for (size_t i = 0; i < result_k; ++i) { A().destroy(sk.data_ + i); } // assumes everything in H region, no gap
-  A().deallocate(sk.data_, sk.curr_items_alloc_);
+  AllocBool(allocator_).deallocate(sk.marks_, sk.curr_items_alloc_);
+  AllocDouble(allocator_).deallocate(sk.weights_, sk.curr_items_alloc_);
+  for (size_t i = 0; i < result_k; ++i) { sk.data_[i].~T(); } // assumes everything in H region, no gap
+  A(allocator_).deallocate(sk.data_, sk.curr_items_alloc_);
 
   sk.data_ = data;
   sk.weights_ = wts;


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org