From 9366531f1d95a6fb742fc98307b1a2cf0c634c0b Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Mon, 16 Dec 2024 13:33:48 -0600
Subject: [PATCH] Remove quadratic behavior cloning rec groups (#1952)

The call to `clone_rec_group` would create a new vector each time it
was called of all clonable rec groups where if an empty rec group is
cloned this creates quadratic behavior where each time a vector is
created and one more item is appended.

The fix in this commit is to avoid creating a precise set of candidates
to clone and just throwing out candidates that aren't applicable.
---
 crates/wasm-smith/src/core.rs | 38 ++++++++++++-----------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/crates/wasm-smith/src/core.rs b/crates/wasm-smith/src/core.rs
index e74e05fabd..1e01785e8b 100644
--- a/crates/wasm-smith/src/core.rs
+++ b/crates/wasm-smith/src/core.rs
@@ -610,7 +610,7 @@ impl Module {
 
         if self.config.gc_enabled {
             // With small probability, clone an existing rec group.
-            if self.clonable_rec_groups(kind).next().is_some() && u.ratio(1, u8::MAX)? {
+            if self.rec_groups.len() > 0 && u.ratio(1, u8::MAX)? {
                 return self.clone_rec_group(u, kind);
             }
 
@@ -640,29 +640,19 @@ impl Module {
         Ok(())
     }
 
-    /// Returns an iterator of rec groups that we could currently clone while
-    /// still staying within the max types limit.
-    fn clonable_rec_groups(
-        &self,
-        kind: AllowEmptyRecGroup,
-    ) -> impl Iterator<Item = Range<usize>> + '_ {
-        self.rec_groups
-            .iter()
-            .filter(move |r| {
-                match kind {
-                    AllowEmptyRecGroup::Yes => {}
-                    AllowEmptyRecGroup::No => {
-                        if r.is_empty() {
-                            return false;
-                        }
-                    }
-                }
-                r.end - r.start <= self.config.max_types.saturating_sub(self.types.len())
-            })
-            .cloned()
-    }
-
     fn clone_rec_group(&mut self, u: &mut Unstructured, kind: AllowEmptyRecGroup) -> Result<()> {
+        // Choose an arbitrary rec group to clone, but bail out if the selected
+        // rec group isn't valid to clone. For example if empty groups aren't
+        // allowed and the selected group is empty, or if cloning the rec group
+        // would cause the maximum number of types to be exceeded.
+        let group = u.choose(&self.rec_groups)?.clone();
+        if group.is_empty() && kind == AllowEmptyRecGroup::No {
+            return Ok(());
+        }
+        if group.len() > self.config.max_types.saturating_sub(self.types.len()) {
+            return Ok(());
+        }
+
         // NB: this does *not* guarantee that the cloned rec group will
         // canonicalize the same as the original rec group and be deduplicated.
         // That would require a second pass over the cloned types to rewrite
@@ -670,8 +660,6 @@ impl Module {
         // new rec group. That might make sense to do one day, but for now we
         // don't do it. That also means that we can't mark the new types as
         // "subtypes" of the old types and vice versa.
-        let candidates: Vec<_> = self.clonable_rec_groups(kind).collect();
-        let group = u.choose(&candidates)?.clone();
         let new_rec_group_start = self.types.len();
         for index in group {
             let orig_ty_index = u32::try_from(index).unwrap();