From 96cfe82664c07163930444e835437ea0c44e5332 Mon Sep 17 00:00:00 2001
From: Matt Mastracci <matthew@mastracci.com>
Date: Tue, 27 Feb 2024 20:30:17 -0700
Subject: perf(cli): reduce overhead in test registration (#22552)

- Removes the origin call, since all origins are the same for an isolate
(ie: the main module)
- Collects the `TestDescription`s and sends them all at the same time
inside of an Arc, allowing us to (later on) re-use these instead of
cloning.

Needs a follow-up pass to remove all the cloning, but that's a thread
that is pretty long to pull

---------

Signed-off-by: Matt Mastracci <matthew@mastracci.com>
---
 cli/tools/test/mod.rs | 71 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 11 deletions(-)

(limited to 'cli/tools/test')

diff --git a/cli/tools/test/mod.rs b/cli/tools/test/mod.rs
index f7a7aed06..526f48c21 100644
--- a/cli/tools/test/mod.rs
+++ b/cli/tools/test/mod.rs
@@ -175,6 +175,51 @@ pub struct TestLocation {
   pub column_number: u32,
 }
 
+#[derive(Default)]
+pub(crate) struct TestContainer(
+  TestDescriptions,
+  Vec<v8::Global<v8::Function>>,
+);
+
+impl TestContainer {
+  pub fn register(
+    &mut self,
+    description: TestDescription,
+    function: v8::Global<v8::Function>,
+  ) {
+    self.0.tests.insert(description.id, description);
+    self.1.push(function)
+  }
+
+  pub fn is_empty(&self) -> bool {
+    self.1.is_empty()
+  }
+}
+
+#[derive(Default, Debug)]
+pub struct TestDescriptions {
+  tests: IndexMap<usize, TestDescription>,
+}
+
+impl TestDescriptions {
+  pub fn len(&self) -> usize {
+    self.tests.len()
+  }
+
+  pub fn is_empty(&self) -> bool {
+    self.tests.is_empty()
+  }
+}
+
+impl<'a> IntoIterator for &'a TestDescriptions {
+  type Item = <&'a IndexMap<usize, TestDescription> as IntoIterator>::Item;
+  type IntoIter =
+    <&'a IndexMap<usize, TestDescription> as IntoIterator>::IntoIter;
+  fn into_iter(self) -> Self::IntoIter {
+    (&self.tests).into_iter()
+  }
+}
+
 #[derive(Debug, Clone, PartialEq, Deserialize, Eq, Hash)]
 #[serde(rename_all = "camelCase")]
 pub struct TestDescription {
@@ -335,10 +380,9 @@ pub enum TestStdioStream {
   Stderr,
 }
 
-#[derive(Debug, Clone, Deserialize)]
-#[serde(rename_all = "camelCase")]
+#[derive(Debug)]
 pub enum TestEvent {
-  Register(TestDescription),
+  Register(Arc<TestDescriptions>),
   Plan(TestPlan),
   Wait(usize),
   Output(TestStdioStream, Vec<u8>),
@@ -559,7 +603,7 @@ async fn test_specifier_inner(
 pub fn worker_has_tests(worker: &mut MainWorker) -> bool {
   let state_rc = worker.js_runtime.op_state();
   let state = state_rc.borrow();
-  !state.borrow::<ops::testing::TestContainer>().0.is_empty()
+  !state.borrow::<TestContainer>().is_empty()
 }
 
 /// Yields to tokio to allow async work to process, and then polls
@@ -587,21 +631,23 @@ pub async fn run_tests_for_worker(
   options: &TestSpecifierOptions,
   fail_fast_tracker: &FailFastTracker,
 ) -> Result<(), AnyError> {
-  let (tests, mut sender) = {
+  let (TestContainer(tests, test_functions), mut sender) = {
     let state_rc = worker.js_runtime.op_state();
     let mut state = state_rc.borrow_mut();
     (
-      std::mem::take(&mut state.borrow_mut::<ops::testing::TestContainer>().0),
+      std::mem::take(&mut *state.borrow_mut::<TestContainer>()),
       state.borrow::<TestEventSender>().clone(),
     )
   };
+  let tests: Arc<TestDescriptions> = tests.into();
+  sender.send(TestEvent::Register(tests.clone()))?;
   let unfiltered = tests.len();
   let tests = tests
     .into_iter()
-    .filter(|(d, _)| options.filter.includes(&d.name))
+    .filter(|(_, d)| options.filter.includes(&d.name))
     .collect::<Vec<_>>();
   let (only, no_only): (Vec<_>, Vec<_>) =
-    tests.into_iter().partition(|(d, _)| d.only);
+    tests.into_iter().partition(|(_, d)| d.only);
   let used_only = !only.is_empty();
   let mut tests = if used_only { only } else { no_only };
   if let Some(seed) = options.shuffle {
@@ -637,7 +683,7 @@ pub async fn run_tests_for_worker(
   filter = filter.omit_op(op_id_host_recv_ctrl as _);
   filter = filter.omit_op(op_id_host_recv_message as _);
 
-  for (desc, function) in tests {
+  for ((_, desc), function) in tests.into_iter().zip(test_functions) {
     if fail_fast_tracker.should_stop() {
       break;
     }
@@ -1146,8 +1192,11 @@ pub async fn report_tests(
   while let Some((_, event)) = receiver.recv().await {
     match event {
       TestEvent::Register(description) => {
-        reporter.report_register(&description);
-        tests.insert(description.id, description);
+        for (_, description) in description.into_iter() {
+          reporter.report_register(description);
+          // TODO(mmastrac): We shouldn't need to clone here -- we can reuse the descriptions everywhere
+          tests.insert(description.id, description.clone());
+        }
       }
       TestEvent::Plan(plan) => {
         if !had_plan {
-- 
cgit v1.2.3