Shard by consecutive slices by default

Add --sharding={round-robin,slice} to control this.

Fixes #546

Author: sourcefrog

NEWS.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+- New: `--sharding` option to control how mutants are distributed across multiple machines, with choices of `slice` or `round-robin`.
+
+- Changed: The default sharding strategy is now `slice`; previously it was `round-robin`. Sliced sharding gives each worker better locality of reference due to testing changes to related packages, but may make the runtime more uneven between workers if some packages are slower to test than others.
+
 - Changed: Tree copying now attempts to use reflinks (copy-on-write) for faster copying on supported filesystems (Btrfs, XFS, APFS), with automatic fallback to regular copying.
 
 - Book: Recommend using the `-Zunstable-options --fail-fast` argument to test targets to speed up mutation testing, on recent nightly toolchains.

book/src/shards.md

@@ -33,6 +33,14 @@ Note that the number of shards is set to match the `/8` in the `--shard` argumen
 
 [Sharding works with `--baseline=skip`](baseline.md), to avoid the cost of running the baseline on every shard. But, if you do this, then you must ensure that the tests suite is passing in the baseline, for example by checking it in a previous CI step.
 
+## Sharding algorithm
+
+The `--sharding` command line and config option controls the algorithm by which mutants are distributed across shards.
+
+* `slice` (the default): The first `n / k` mutants are assigned to shard 0, and so on. Because each shard successively builds related versions of the code, incremental builds may be faster, particularly in trees with many packages.
+
+* `round-robin`: Mutant `i` is assigned to shard `i % n`. This distributes the mutants evenly across shards and is likely to cause shards to finish at similar times.
+
 ## Performance of sharding
 
 Each mutant does some constant upfront work:

src/main.rs

@@ -560,7 +560,7 @@ fn main() -> Result<()> {
         };
     }
     if let Some(shard) = &args.shard {
-        mutants = shard.select(mutants);
+        mutants = options.sharding().shard(*shard, mutants);
     }
     if args.list {
         print!("{}", list_mutants(&mutants, &options));

src/options.rs

@@ -28,6 +28,7 @@ use crate::annotation::ResolvedAnnotation;
 use crate::config::Config;
 use crate::glob::build_glob_set;
 use crate::mutant::Mutant;
+use crate::shard::Sharding;
 use crate::{Args, BaselineStrategy, Phase, Result, ValueEnum};
 
 /// Options for mutation testing, based on both command-line arguments and the config file.
@@ -180,16 +181,22 @@ pub struct Common {
     /// Tool used to run test suites: cargo or nextest.
     #[arg(long, help_heading = "Execution")]
     pub test_tool: Option<TestTool>,
+
+    /// Sharding method to use when running with --shards.
+    #[arg(long, help_heading = "Execution", requires = "shard")]
+    pub sharding: Option<Sharding>,
 }
 
 impl Common {
     /// Merge two `MoreOptions`, taking values from self first.
     ///
     /// Scalar values are taken from self if present, otherwise from other.
+    #[allow(clippy::trivially_copy_pass_by_ref)] // will be bigger later
     pub fn merge(&self, other: &Common) -> Common {
         Common {
             emit_diffs: self.emit_diffs.or(other.emit_diffs),
             test_tool: self.test_tool.or(other.test_tool),
+            sharding: self.sharding.or(other.sharding),
         }
     }
 }
@@ -473,6 +480,10 @@ impl Options {
     pub fn test_tool(&self) -> TestTool {
         self.common.test_tool.unwrap_or_default()
     }
+
+    pub fn sharding(&self) -> Sharding {
+        self.common.sharding.unwrap_or_default()
+    }
 }
 
 /// If the first slices is non-empty, return that, otherwise the second.
@@ -506,6 +517,8 @@ mod test {
         assert!(options.common.test_tool.is_none());
         assert_eq!(options.test_tool(), TestTool::Cargo);
         assert!(!options.cap_lints);
+        assert!(options.common.sharding.is_none());
+        assert_eq!(options.sharding(), Sharding::Slice);
     }
 
     #[test]
@@ -1132,4 +1145,37 @@ mod test {
         let options = Options::new(&args, &config).unwrap();
         assert!(!options.copy_vcs);
     }
+
+    #[test]
+    fn sharding() {
+        let args = Args::try_parse_from(["mutants", "--sharding=slice", "--shard=0/10"]).unwrap();
+        let config = Config::default();
+        let options = Options::new(&args, &config).unwrap();
+        assert_eq!(options.sharding(), Sharding::Slice);
+
+        let args = Args::parse_from(["mutants", "--sharding=round-robin", "--shard=0/10"]);
+        let config = Config::default();
+        let options = Options::new(&args, &config).unwrap();
+        assert_eq!(options.sharding(), Sharding::RoundRobin);
+
+        let args = Args::parse_from(["mutants"]);
+        let config = Config::from_str(r#"sharding = "slice""#).unwrap();
+        let options = Options::new(&args, &config).unwrap();
+        assert_eq!(options.sharding(), Sharding::Slice);
+
+        let args = Args::parse_from(["mutants"]);
+        let config = Config::from_str(r#"sharding = "round-robin""#).unwrap();
+        dbg!(&config);
+        let options = Options::new(&args, &config).unwrap();
+        assert_eq!(options.sharding(), Sharding::RoundRobin);
+
+        let args = Args::parse_from(["mutants"]);
+        let config = Config::from_str("").unwrap();
+        let options = Options::new(&args, &config).unwrap();
+        assert_eq!(
+            options.sharding(),
+            Sharding::Slice,
+            "Default sharding should be slice"
+        );
+    }
 }

src/shard.rs

@@ -1,10 +1,11 @@
-// Copyright 2023 Martin Pool
-
 //! Sharding parameters.
 
 use std::str::FromStr;
 
 use anyhow::{anyhow, ensure, Context, Error};
+use clap::ValueEnum;
+use schemars::JsonSchema;
+use serde::Deserialize;
 
 /// Select mutants for a particular shard of the total list.
 #[derive(Debug, Clone, Copy, Eq, PartialEq)]
@@ -15,17 +16,6 @@ pub struct Shard {
     pub n: usize,
 }
 
-impl Shard {
-    /// Select the mutants that should be run for this shard.
-    pub fn select<M, I: IntoIterator<Item = M>>(&self, mutants: I) -> Vec<M> {
-        mutants
-            .into_iter()
-            .enumerate()
-            .filter_map(|(i, m)| if i % self.n == self.k { Some(m) } else { None })
-            .collect()
-    }
-}
-
 impl FromStr for Shard {
     type Err = Error;
 
@@ -38,6 +28,57 @@ impl FromStr for Shard {
     }
 }
 
+/// Method for sharding.
+#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Deserialize, ValueEnum, JsonSchema)]
+#[serde(rename_all = "kebab-case")] // consistent with Clap default
+pub enum Sharding {
+    /// Run mutant `i` on shard `i % k`.
+    ///
+    /// This was the default up to cargo-mutants 25.3.1.
+    ///
+    /// This distributes mutants more evenly and will likely generate more equal completion
+    /// times, but it has less locality of reference within each shard's cache and so may
+    /// cause more build time.
+    RoundRobin,
+
+    /// Run consecutive ranges of mutants on each shard: the first `n/k` on shard 0, etc. (default)
+    ///
+    /// This makes the incremental change between each build likely to be smaller and
+    /// so may reduce build time, but it may also lead to more unbalanced shards.
+    #[default]
+    Slice,
+}
+
+impl Sharding {
+    /// Select the mutants that should be run for this shard.
+    pub fn shard<M>(self, shard: Shard, mut mutants: Vec<M>) -> Vec<M> {
+        match self {
+            Sharding::RoundRobin => mutants
+                .into_iter()
+                .enumerate()
+                .filter_map(|(i, m)| {
+                    if i % shard.n == shard.k {
+                        Some(m)
+                    } else {
+                        None
+                    }
+                })
+                .collect(),
+            Sharding::Slice => {
+                let total = mutants.len();
+                let chunk_size = total.div_ceil(shard.n);
+                let start = shard.k * chunk_size;
+                let end = ((shard.k + 1) * chunk_size).min(total);
+                if start >= total {
+                    Vec::new()
+                } else {
+                    mutants.drain(start..end).collect()
+                }
+            }
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -74,10 +115,21 @@ mod tests {
     }
 
     #[test]
-    fn shard_select() {
-        assert_eq!(
-            Shard::from_str("1/4").unwrap().select(0..10).as_slice(),
-            &[1, 5, 9]
-        );
+    fn shard_round_robin() {
+        // This test works on ints instead of real mutants just for ease of testing.
+        let fake_mutants: Vec<usize> = (0..10).collect();
+        for (k, expect) in [
+            (0, [0, 4, 8].as_slice()),
+            (1, &[1, 5, 9]),
+            (2, &[2, 6]),
+            (3, &[3, 7]),
+        ] {
+            assert_eq!(
+                Sharding::RoundRobin
+                    .shard(Shard { k, n: 4 }, fake_mutants.clone())
+                    .as_slice(),
+                expect
+            );
+        }
     }
 }

tests/shard.rs

@@ -28,12 +28,16 @@ fn shard_divides_all_mutants() {
     .collect_vec();
 
     let n_shards = 5;
-    let shard_lists = (0..n_shards)
+    let rr_shard_lists = (0..n_shards)
         .map(|k| {
             String::from_utf8(
                 run()
                     .args(common_args)
-                    .args(["--shard", &format!("{k}/{n_shards}")])
+                    .args([
+                        "--shard",
+                        &format!("{k}/{n_shards}"),
+                        "--sharding=round-robin",
+                    ])
                     .assert()
                     .success()
                     .get_output()
@@ -53,28 +57,73 @@ fn shard_divides_all_mutants() {
     // If we had a bug where we shuffled before sharding, then the shards would
     // see inconsistent lists and this test would fail in at least some attempts.
     assert_eq!(
-        shard_lists.iter().flatten().sorted().collect_vec(),
+        rr_shard_lists.iter().flatten().sorted().collect_vec(),
         full_list.iter().sorted().collect_vec()
     );
 
     // The shards should also be approximately the same size.
-    let shard_lens = shard_lists.iter().map(|l| l.len()).collect_vec();
+    let shard_lens = rr_shard_lists.iter().map(|l| l.len()).collect_vec();
     assert!(shard_lens.iter().max().unwrap() - shard_lens.iter().min().unwrap() <= 1);
 
     // And the shards are disjoint
     for i in 0..n_shards {
         for j in 0..n_shards {
             if i != j {
                 assert!(
-                    shard_lists[i].iter().all(|m| !shard_lists[j].contains(m)),
+                    rr_shard_lists[i]
+                        .iter()
+                        .all(|m| !rr_shard_lists[j].contains(m)),
                     "shard {} contains {}",
                     j,
-                    shard_lists[j]
+                    rr_shard_lists[j]
                         .iter()
-                        .filter(|m| shard_lists[i].contains(m))
+                        .filter(|m| rr_shard_lists[j].contains(m))
                         .join(", ")
                 );
             }
         }
     }
+
+    // If you reassemble the round-robin shards in order, you get the original order back.
+    //
+    // To do so: cycle around the list of shards, taking one from each shard in order, until
+    // we get to the end of any list.
+    let mut reassembled = Vec::new();
+    let mut rr_iters = rr_shard_lists
+        .clone()
+        .into_iter()
+        .map(|l| l.into_iter())
+        .collect_vec();
+    let mut i = 0;
+    let mut limit = 0;
+    for name in rr_iters[i].by_ref() {
+        reassembled.push(name);
+        i = (i + 1) % n_shards;
+        limit += 1;
+        assert!(limit < full_list.len() * 2, "too many iterations");
+    }
+
+    // Check with slice sharding, the new default
+    let slice_shard_lists = (0..n_shards)
+        .map(|k| {
+            String::from_utf8(
+                run()
+                    .args(common_args)
+                    .args([&format!("--shard={k}/{n_shards}")]) //  "--sharding=slice"
+                    .assert()
+                    .success()
+                    .get_output()
+                    .stdout
+                    .clone(),
+            )
+            .unwrap()
+            .lines()
+            .map(ToOwned::to_owned)
+            .collect_vec()
+        })
+        .collect_vec();
+
+    // These can just be concatenated
+    let slice_reassembled = slice_shard_lists.into_iter().flatten().collect_vec();
+    assert_eq!(slice_reassembled, full_list);
 }