perf: avoid clone on serialize frequency items

[tisonkun]

cc @k3dom

[k3dom]

Nice, LGTM!

Two small thoughts:

• active_entries() could probably be a lazy iterator impl Iterator<Item = (&T, u64)> + '_ instead of collecting Vec<(&T, u64)> which would make it totally allocation-free, not sure if it would be always faster though for very large maps.
• Would probably also make sense to update frequent_items_with_threshold as a follow-up because here we still clone every qualifying item into owned Row's before the caller can filter

[tisonkun]

@k3dom

active_entries() could probably be a lazy iterator impl Iterator<Item = (&T, u64)> + '_ instead of collecting Vec<(&T, u64)> which would make it totally allocation-free, not sure if it would be always faster though for very large maps.

This is proposed by my Codex for the first time. But I reject it because an active_entries iterator would do the activity check every time, while we'd iterate it several (now 3) times. So it's not a net win.

Would probably also make sense to update frequent_items_with_threshold as a follow-up because here we still clone every qualifying item into owned Row's before the caller can filter

This may be possible. But now the returned rows are sorted so it seems not to be suitable to return an iterator.

If you mean we may change Row<T> to Row<'a, T>, as it's used by end users, I'm afraid that would increase usage complexity - we may review this when we have a concrete use case that proves its benefits.

[tisonkun]

But Row<'a, T> is possible, anyway. I just need one more consideration and would prefer a real use case.

Something like:

index 49dce3b..6895e4a 100644
--- a/datasketches/src/frequencies/sketch.rs
+++ b/datasketches/src/frequencies/sketch.rs
@@ -57,17 +57,17 @@ pub enum ErrorType {
 ///
 /// Each row includes an estimate and upper and lower bounds on the true frequency.
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub struct Row<T> {
-    item: T,
+pub struct Row<'a, T> {
+    item: &'a T,
     estimate: u64,
     upper_bound: u64,
     lower_bound: u64,
 }
 
-impl<T> Row<T> {
+impl<'a, T> Row<'a, T> {
     /// Returns the item value.
-    pub fn item(&self) -> &T {
-        &self.item
+    pub fn item(&self) -> &'a T {
+        self.item
     }
 
     /// Returns the estimated frequency.
@@ -378,10 +378,7 @@ impl<T: Eq + Hash> FrequentItemsSketch<T> {
     /// let rows = sketch.frequent_items(ErrorType::NoFalseNegatives);
     /// assert!(rows.iter().any(|row| *row.item() == 1));
     /// ```
-    pub fn frequent_items(&self, error_type: ErrorType) -> Vec<Row<T>>
-    where
-        T: Clone,
-    {
+    pub fn frequent_items(&self, error_type: ErrorType) -> impl Iterator<Item = Row<'_, T>> {
         self.frequent_items_with_threshold(error_type, self.offset)
     }
 
@@ -407,13 +404,9 @@ impl<T: Eq + Hash> FrequentItemsSketch<T> {
         &self,
         error_type: ErrorType,
         threshold: u64,
-    ) -> Vec<Row<T>>
-    where
-        T: Clone,
-    {
+    ) -> impl Iterator<Item = Row<'_, T>> {
         let threshold = threshold.max(self.offset);
-        let mut rows = vec![];
-        for (item, count) in self.hash_map.iter() {
+        self.hash_map.iter().filter_map(move |(item, count)| {
             let lower = count;
             let upper = count + self.offset;
             let include = match error_type {
@@ -421,16 +414,16 @@ impl<T: Eq + Hash> FrequentItemsSketch<T> {
                 ErrorType::NoFalsePositives => lower > threshold,
             };
             if include {
-                rows.push(Row {
-                    item: item.clone(),
+                Some(Row {
+                    item,
                     estimate: upper,
                     upper_bound: upper,
                     lower_bound: lower,
-                });
+                })
+            } else {
+                None
             }
-        }
-        rows.sort_by_key(|row| std::cmp::Reverse(row.estimate));
-        rows
+        })
     }
 
     fn maybe_resize_or_purge(&mut self) {

But this would drop the sorted character that requires users to sort on their own. I need to review the contract.

[k3dom]

This is proposed by my Codex at the first time. But I reject it because an active_entries iterator would do the activity check every time while we'd iterator it several (now 3) times. So it's not a net win.

Yeah agree, that is what I meant when I said it's probably not faster for large maps. For most map sizes the three scans should be cheap but sparse scans probably loose to collect-once.

But Row<'a, T> is possible, anyway. I just need one more consideration and would prefer a real use case.

Agree, I don't have a use case for my project right now either, just thought I mention it because I noticed it while going over the API once again