From c940205353bd4401eecc757c7b18f12cfbdc5878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Sun, 24 Mar 2024 05:22:37 +0000 Subject: [PATCH] refactor(bench): align ops to testing ops (#23038) Internal refactor that changes how we use ops in `deno bench` subcommand. This brings it in line to what we do in `deno test` subcommand. --- cli/js/40_bench.js | 54 ++++++++++++++++++++++++++------ cli/ops/bench.rs | 71 ++++++++++++++++++++----------------------- runtime/js/99_main.js | 1 + 3 files changed, 78 insertions(+), 48 deletions(-) diff --git a/cli/js/40_bench.js b/cli/js/40_bench.js index b7a93038c0..e1373c990e 100644 --- a/cli/js/40_bench.js +++ b/cli/js/40_bench.js @@ -9,7 +9,12 @@ import { } from "ext:cli/40_test_common.js"; import { Console } from "ext:deno_console/01_console.js"; import { setExitHandler } from "ext:runtime/30_os.js"; -const ops = core.ops; +const { + op_register_bench, + op_bench_get_origin, + op_dispatch_bench_event, + op_bench_now, +} = core.ops; const { ArrayPrototypePush, Error, @@ -30,6 +35,12 @@ let currentBenchUserExplicitEnd = null; let registeredWarmupBench = false; +const registerBenchIdRetBuf = new Uint32Array(1); +const registerBenchIdRetBufU8 = new Uint8Array(registerBenchIdRetBuf.buffer); + +// As long as we're using one isolate per test, we can cache the origin since it won't change +let cachedOrigin = undefined; + // Main bench function provided by Deno. function bench( nameOrFnOrOptions, @@ -49,10 +60,22 @@ function bench( permissions: null, warmup: true, }; + if (cachedOrigin == undefined) { + cachedOrigin = op_bench_get_origin(); + } warmupBenchDesc.fn = wrapBenchmark(warmupBenchDesc); - const { id, origin } = ops.op_register_bench(warmupBenchDesc); - warmupBenchDesc.id = id; - warmupBenchDesc.origin = origin; + op_register_bench( + warmupBenchDesc.fn, + warmupBenchDesc.name, + warmupBenchDesc.baseline, + warmupBenchDesc.group, + warmupBenchDesc.ignore, + warmupBenchDesc.only, + warmupBenchDesc.warmup, + registerBenchIdRetBufU8, + ); + warmupBenchDesc.id = registerBenchIdRetBufU8[0]; + warmupBenchDesc.origin = cachedOrigin; } let benchDesc; @@ -139,10 +162,21 @@ function bench( benchDesc.fn = wrapBenchmark(benchDesc); benchDesc.warmup = false; benchDesc.name = escapeName(benchDesc.name); - - const { id, origin } = ops.op_register_bench(benchDesc); - benchDesc.id = id; - benchDesc.origin = origin; + if (cachedOrigin == undefined) { + cachedOrigin = op_bench_get_origin(); + } + op_register_bench( + benchDesc.fn, + benchDesc.name, + benchDesc.baseline, + benchDesc.group, + benchDesc.ignore, + benchDesc.only, + false, + registerBenchIdRetBufU8, + ); + benchDesc.id = registerBenchIdRetBufU8[0]; + benchDesc.origin = cachedOrigin; } function compareMeasurements(a, b) { @@ -381,7 +415,7 @@ function wrapBenchmark(desc) { try { globalThis.console = new Console((s) => { - ops.op_dispatch_bench_event({ output: s }); + op_dispatch_bench_event({ output: s }); }); if (desc.permissions) { @@ -420,7 +454,7 @@ function wrapBenchmark(desc) { } function benchNow() { - return ops.op_bench_now(); + return op_bench_now(); } globalThis.Deno.bench = bench; diff --git a/cli/ops/bench.rs b/cli/ops/bench.rs index 2e704a49d9..2c8c63ed66 100644 --- a/cli/ops/bench.rs +++ b/cli/ops/bench.rs @@ -5,17 +5,15 @@ use std::sync::atomic::Ordering; use std::time; use deno_core::error::generic_error; +use deno_core::error::type_error; use deno_core::error::AnyError; use deno_core::op2; -use deno_core::serde_v8; use deno_core::v8; use deno_core::ModuleSpecifier; use deno_core::OpState; use deno_runtime::permissions::create_child_permissions; use deno_runtime::permissions::ChildPermissionsArg; use deno_runtime::permissions::PermissionsContainer; -use serde::Deserialize; -use serde::Serialize; use tokio::sync::mpsc::UnboundedSender; use uuid::Uuid; @@ -32,6 +30,7 @@ deno_core::extension!(deno_bench, op_pledge_test_permissions, op_restore_test_permissions, op_register_bench, + op_bench_get_origin, op_dispatch_bench_event, op_bench_now, ], @@ -44,6 +43,12 @@ deno_core::extension!(deno_bench, }, ); +#[op2] +#[string] +fn op_bench_get_origin(state: &mut OpState) -> String { + state.borrow::().to_string() +} + #[derive(Clone)] struct PermissionsHolder(Uuid, PermissionsContainer); @@ -94,57 +99,47 @@ pub fn op_restore_test_permissions( } } -#[derive(Deserialize)] -#[serde(rename_all = "camelCase")] -struct BenchInfo<'s> { - #[serde(rename = "fn")] - function: serde_v8::Value<'s>, - name: String, - baseline: bool, - group: Option, - ignore: bool, - only: bool, - #[serde(default)] - warmup: bool, -} - -#[derive(Debug, Serialize)] -#[serde(rename_all = "camelCase")] -struct BenchRegisterResult { - id: usize, - origin: String, -} - static NEXT_ID: AtomicUsize = AtomicUsize::new(0); +#[allow(clippy::too_many_arguments)] #[op2] -#[serde] -fn op_register_bench<'a>( - scope: &mut v8::HandleScope<'a>, +fn op_register_bench( state: &mut OpState, - #[serde] info: BenchInfo<'a>, -) -> Result { + #[global] function: v8::Global, + #[string] name: String, + baseline: bool, + #[string] group: Option, + ignore: bool, + only: bool, + warmup: bool, + #[buffer] ret_buf: &mut [u8], +) -> Result<(), AnyError> { + if ret_buf.len() != 4 { + return Err(type_error(format!( + "Invalid ret_buf length: {}", + ret_buf.len() + ))); + } let id = NEXT_ID.fetch_add(1, Ordering::SeqCst); let origin = state.borrow::().to_string(); let description = BenchDescription { id, - name: info.name, + name, origin: origin.clone(), - baseline: info.baseline, - group: info.group, - ignore: info.ignore, - only: info.only, - warmup: info.warmup, + baseline, + group, + ignore, + only, + warmup, }; - let function: v8::Local = info.function.v8_value.try_into()?; - let function = v8::Global::new(scope, function); state .borrow_mut::() .0 .push((description.clone(), function)); let sender = state.borrow::>().clone(); sender.send(BenchEvent::Register(description)).ok(); - Ok(BenchRegisterResult { id, origin }) + ret_buf.copy_from_slice(&(id as u32).to_le_bytes()); + Ok(()) } #[op2] diff --git a/runtime/js/99_main.js b/runtime/js/99_main.js index a47a7163e1..3db781148b 100644 --- a/runtime/js/99_main.js +++ b/runtime/js/99_main.js @@ -564,6 +564,7 @@ const NOT_IMPORTED_OPS = [ "op_bench_now", "op_dispatch_bench_event", "op_register_bench", + "op_bench_get_origin", // Related to `Deno.jupyter` API "op_jupyter_broadcast",