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/js/40_test.js

@@ -11,6 +11,7 @@ const {
   op_test_event_step_result_ignored,
   op_test_event_step_result_ok,
   op_test_event_step_wait,
+  op_test_get_origin,
 } = core.ops;
 const {
   ArrayPrototypeFilter,
@@ -188,6 +189,9 @@ function wrapInner(fn) {
 const registerTestIdRetBuf = new Uint32Array(1);
 const registerTestIdRetBufU8 = new Uint8Array(registerTestIdRetBuf.buffer);
 
+// As long as we're using one isolate per test, we can cache the origin since it won't change
+let cachedOrigin = undefined;
+
 function testInner(
   nameOrFnOrOptions,
   optionsOrFn,
@@ -279,11 +283,15 @@ function testInner(
   // Delete this prop in case the user passed it. It's used to detect steps.
   delete testDesc.parent;
 
+  if (cachedOrigin == undefined) {
+    cachedOrigin = op_test_get_origin();
+  }
+
   testDesc.location = core.currentUserCallSite();
   testDesc.fn = wrapTest(testDesc);
   testDesc.name = escapeName(testDesc.name);
 
-  const origin = op_register_test(
+  op_register_test(
     testDesc.fn,
     testDesc.name,
     testDesc.ignore,
@@ -296,7 +304,7 @@ function testInner(
     registerTestIdRetBufU8,
   );
   testDesc.id = registerTestIdRetBuf[0];
-  testDesc.origin = origin;
+  testDesc.origin = cachedOrigin;
   MapPrototypeSet(testStates, testDesc.id, {
     context: createTestContext(testDesc),
     children: [],

cli/lsp/testing/execution.rs

@@ -324,8 +324,11 @@ impl TestRun {
         while let Some((_, event)) = receiver.recv().await {
           match event {
             test::TestEvent::Register(description) => {
-              reporter.report_register(&description);
+              for (_, description) in description.into_iter() {
-              tests.write().insert(description.id, description);
+                reporter.report_register(description);
+                // TODO(mmastrac): we shouldn't need to clone here - we can re-use the descriptions
+                tests.write().insert(description.id, description.clone());
+              }
             }
             test::TestEvent::Plan(plan) => {
               summary.total += plan.total;

cli/ops/testing.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
+use crate::tools::test::TestContainer;
 use crate::tools::test::TestDescription;
 use crate::tools::test::TestEvent;
 use crate::tools::test::TestEventSender;
@@ -23,17 +24,13 @@ use std::sync::atomic::AtomicUsize;
 use std::sync::atomic::Ordering;
 use uuid::Uuid;
 
-#[derive(Default)]
-pub(crate) struct TestContainer(
-  pub Vec<(TestDescription, v8::Global<v8::Function>)>,
-);
-
 deno_core::extension!(deno_test,
   ops = [
     op_pledge_test_permissions,
     op_restore_test_permissions,
     op_register_test,
     op_register_test_step,
+    op_test_get_origin,
     op_test_event_step_wait,
     op_test_event_step_result_ok,
     op_test_event_step_result_ignored,
@@ -106,7 +103,6 @@ static NEXT_ID: AtomicUsize = AtomicUsize::new(0);
 
 #[allow(clippy::too_many_arguments)]
 #[op2]
-#[string]
 fn op_register_test(
   state: &mut OpState,
   #[global] function: v8::Global<v8::Function>,
@@ -119,7 +115,7 @@ fn op_register_test(
   #[smi] line_number: u32,
   #[smi] column_number: u32,
   #[buffer] ret_buf: &mut [u8],
-) -> Result<String, AnyError> {
+) -> Result<(), AnyError> {
   if ret_buf.len() != 4 {
     return Err(type_error(format!(
       "Invalid ret_buf length: {}",
@@ -142,14 +138,16 @@ fn op_register_test(
       column_number,
     },
   };
-  state
+  let container = state.borrow_mut::<TestContainer>();
-    .borrow_mut::<TestContainer>()
+  container.register(description, function);
-    .0
-    .push((description.clone(), function));
-  let sender = state.borrow_mut::<TestEventSender>();
-  sender.send(TestEvent::Register(description)).ok();
   ret_buf.copy_from_slice(&(id as u32).to_le_bytes());
-  Ok(origin)
+  Ok(())
+}
+
+#[op2]
+#[string]
+fn op_test_get_origin(state: &mut OpState) -> String {
+  state.borrow::<ModuleSpecifier>().to_string()
 }
 
 #[op2(fast)]

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)]
+#[derive(Debug)]
-#[serde(rename_all = "camelCase")]
 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);
+        for (_, description) in description.into_iter() {
-        tests.insert(description.id, description);
+          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 {

runtime/js/99_main.js

@@ -578,6 +578,7 @@ const NOT_IMPORTED_OPS = [
   "op_restore_test_permissions",
   "op_register_test_step",
   "op_register_test",
+  "op_test_get_origin",
   "op_pledge_test_permissions",
 
   // TODO(bartlomieju): used in various integration tests - figure out a way

tests/unit/ops_test.ts

@@ -1,6 +1,6 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
-const EXPECTED_OP_COUNT = 11;
+const EXPECTED_OP_COUNT = 12;
 
 Deno.test(function checkExposedOps() {
   // @ts-ignore TS doesn't allow to index with symbol