Change parameters to exert_logic from iterator to slice (#461)

* Change parameters to exert_logic from iterator to slice

This avoid the intermediate allocation for every invocation of exert_logic
to create a boxed iterator. Instead, pass a slice that contains the same
information previously revealed by the iterator. At the same time, wrap
the logic in an option to only invoke it if it is set.

Signed-off-by: Moritz Hoffmann <[email protected]>

* Update CI

Signed-off-by: Moritz Hoffmann <[email protected]>

---------

Signed-off-by: Moritz Hoffmann <[email protected]>

.github/workflows/test.yml

@@ -1,18 +1,43 @@
-name: test
-
-on: [push, pull_request]
+name: "Test Suite"
+on:
+  push:
+  pull_request:
 
 jobs:
   test:
-    runs-on: ubuntu-22.04
+    strategy:
+      matrix:
+        os:
+          - ubuntu
+          - macos
+          - windows
+        toolchain:
+          - stable
+          - 1.72
+    name: cargo test on ${{ matrix.os }}
+    runs-on: ${{ matrix.os }}-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions-rust-lang/setup-rust-toolchain@v1
+        with:
+          toolchain: ${{ matrix.toolchain }}
+      - name: Cargo test
+        run: cargo test
+
+  # Check formatting with rustfmt
+  mdbook:
+    name: test mdBook
+    runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v3
-    - run: rustup update 1.70 --no-self-update && rustup default 1.70
-    - run: cargo build --workspace
-    - name: test mdBook
-      # rustdoc doesn't build dependencies, so it needs to run after `cargo build`,
-      # but its dependency search gets confused if there are multiple copies of any
-      # dependency in target/debug/deps, so it needs to run before `cargo test` et al.
-      # clutter target/debug/deps with multiple copies of things.
-      run: for file in $(find mdbook -name '*.md' | sort); do rustdoc --test $file  -L ./target/debug/deps; done
-    - run: cargo test --workspace
+      - uses: actions/checkout@v4
+      # Ensure rustfmt is installed and setup problem matcher
+      - uses: actions-rust-lang/setup-rust-toolchain@v1
+        with:
+          components: rustfmt
+      - run: cargo build
+      - name: test mdBook
+        # rustdoc doesn't build dependencies, so it needs to run after `cargo build`,
+        # but its dependency search gets confused if there are multiple copies of any
+        # dependency in target/debug/deps, so it needs to run before `cargo test` et al.
+        # clutter target/debug/deps with multiple copies of things.
+        run: for file in $(find mdbook -name '*.md' | sort); do rustdoc --test $file  -L ./target/debug/deps; done

src/lib.rs

@@ -135,8 +135,8 @@ pub fn configure(config: &mut timely::WorkerConfig, options: &Config) {
             std::sync::Arc::new(move |batches| {
                 let mut non_empty = 0;
                 for (_index, count, length) in batches {
-                    if count > 1 { return Some(effort as usize); }
-                    if length > 0 { non_empty += 1; }
+                    if *count > 1 { return Some(effort as usize); }
+                    if *length > 0 { non_empty += 1; }
                     if non_empty > 1 { return Some(effort as usize); }
                 }
                 None

src/trace/implementations/spine_fueled.rs

@@ -98,8 +98,10 @@ where
     upper: Antichain<B::Time>,
     effort: usize,
     activator: Option<timely::scheduling::activate::Activator>,
+    /// Parameters to `exert_logic`, containing tuples of `(index, count, length)`.
+    exert_logic_param: Vec<(usize, usize, usize)>,
     /// Logic to indicate whether and how many records we should introduce in the absence of actual updates.
-    exert_logic: ExertionLogic,
+    exert_logic: Option<ExertionLogic>,
     phantom: std::marker::PhantomData<(BA, BU)>,
 }
 
@@ -295,7 +297,7 @@ where
     }
 
     fn set_exert_logic(&mut self, logic: ExertionLogic) {
-        self.exert_logic = logic;
+        self.exert_logic = Some(logic);
     }
 
     // Ideally, this method acts as insertion of `batch`, even if we are not yet able to begin
@@ -383,16 +385,19 @@ impl<B: Batch, BA, BU> Spine<B, BA, BU> {
     ///
     /// This method prepares an iterator over batches, including the level, count, and length of each layer.
     /// It supplies this to `self.exert_logic`, who produces the response of the amount of exertion to apply.
-    fn exert_effort(&self) -> Option<usize> {
-        (self.exert_logic)(
-            Box::new(self.merging.iter().enumerate().rev().map(|(index, batch)| {
+    fn exert_effort(&mut self) -> Option<usize> {
+        self.exert_logic.as_ref().and_then(|exert_logic| {
+            self.exert_logic_param.clear();
+            self.exert_logic_param.extend(self.merging.iter().enumerate().rev().map(|(index, batch)| {
                 match batch {
                     MergeState::Vacant => (index, 0, 0),
                     MergeState::Single(_) => (index, 1, batch.len()),
                     MergeState::Double(_) => (index, 2, batch.len()),
                 }
-            }))
-        )
+            }));
+
+            (exert_logic)(&self.exert_logic_param[..])
+        })
     }
 
     /// Describes the merge progress of layers in the trace.
@@ -435,7 +440,8 @@ impl<B: Batch, BA, BU> Spine<B, BA, BU> {
             upper: Antichain::from_elem(<B::Time as timely::progress::Timestamp>::minimum()),
             effort,
             activator,
-            exert_logic: std::sync::Arc::new(|_batches| None),
+            exert_logic_param: Vec::default(),
+            exert_logic: None,
             phantom: std::marker::PhantomData,
         }
     }

src/trace/mod.rs

@@ -27,7 +27,7 @@ pub use self::cursor::Cursor;
 pub use self::description::Description;
 
 /// A type used to express how much effort a trace should exert even in the absence of updates.
-pub type ExertionLogic = std::sync::Arc<dyn for<'a> Fn(Box<dyn Iterator<Item=(usize, usize, usize)>+'a>)->Option<usize>+Send+Sync>;
+pub type ExertionLogic = std::sync::Arc<dyn for<'a> Fn(&'a [(usize, usize, usize)])->Option<usize>+Send+Sync>;
 
 //     The traces and batch and cursors want the flexibility to appear as if they manage certain types of keys and
 //     values and such, while perhaps using other representations, I'm thinking mostly of wrappers around the keys