From 39c7d8dafe00fd619afac7de0151790e7d53cd43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Sun, 3 Dec 2023 03:07:04 +0100 Subject: [PATCH] refactor: faster args for op_load in TSC (#21438) This commit changes the argument that "op_load" accepts, from a serde struct to "&str". This should equal to a slightly better performance. --- cli/build.rs | 17 ++++------- cli/lsp/tsc.rs | 14 ++++----- cli/tsc/99_main_compiler.js | 6 ++-- cli/tsc/mod.rs | 59 +++++++++++-------------------------- 4 files changed, 29 insertions(+), 67 deletions(-) diff --git a/cli/build.rs b/cli/build.rs index edb2e4fdd3..4400e3e8b3 100644 --- a/cli/build.rs +++ b/cli/build.rs @@ -13,18 +13,11 @@ mod ts { use deno_core::op2; use deno_core::OpState; use deno_runtime::deno_node::SUPPORTED_BUILTIN_NODE_MODULES; - use serde::Deserialize; use serde::Serialize; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; - #[derive(Debug, Deserialize)] - struct LoadArgs { - /// The fully qualified specifier that should be loaded. - specifier: String, - } - #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] struct BuildInfoResponse { @@ -81,7 +74,7 @@ mod ts { // files, but a slightly different implementation at build time. fn op_load( state: &mut OpState, - #[serde] args: LoadArgs, + #[string] load_specifier: &str, ) -> Result { let op_crate_libs = state.borrow::>(); let path_dts = state.borrow::(); @@ -89,7 +82,7 @@ mod ts { let build_specifier = "asset:///bootstrap.ts"; // we need a basic file to send to tsc to warm it up. - if args.specifier == build_specifier { + if load_specifier == build_specifier { Ok(LoadResponse { data: r#"Deno.writeTextFile("hello.txt", "hello deno!");"#.to_string(), version: "1".to_string(), @@ -98,7 +91,7 @@ mod ts { }) // specifiers come across as `asset:///lib.{lib_name}.d.ts` and we need to // parse out just the name so we can lookup the asset. - } else if let Some(caps) = re_asset.captures(&args.specifier) { + } else if let Some(caps) = re_asset.captures(load_specifier) { if let Some(lib) = caps.get(1).map(|m| m.as_str()) { // if it comes from an op crate, we were supplied with the path to the // file. @@ -118,13 +111,13 @@ mod ts { } else { Err(custom_error( "InvalidSpecifier", - format!("An invalid specifier was requested: {}", args.specifier), + format!("An invalid specifier was requested: {}", load_specifier), )) } } else { Err(custom_error( "InvalidSpecifier", - format!("An invalid specifier was requested: {}", args.specifier), + format!("An invalid specifier was requested: {}", load_specifier), )) } } diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index b4e02394bf..210ef701a5 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -3835,12 +3835,6 @@ impl State { } } -#[derive(Debug, Deserialize, Serialize)] -#[serde(rename_all = "camelCase")] -struct SpecifierArgs { - specifier: String, -} - #[op2(fast)] fn op_is_cancelled(state: &mut OpState) -> bool { let state = state.borrow_mut::(); @@ -3873,11 +3867,13 @@ struct LoadResponse { fn op_load<'s>( scope: &'s mut v8::HandleScope, state: &mut OpState, - #[serde] args: SpecifierArgs, + #[string] specifier: &str, ) -> Result, AnyError> { let state = state.borrow_mut::(); - let mark = state.performance.mark_with_args("tsc.op.op_load", &args); - let specifier = state.specifier_map.normalize(args.specifier)?; + let mark = state + .performance + .mark_with_args("tsc.op.op_load", specifier); + let specifier = state.specifier_map.normalize(specifier)?; let maybe_load_response = if specifier.as_str() == "internal:///missing_dependency.d.ts" { Some(LoadResponse { diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js index b72a3b0efa..3f67ccb241 100644 --- a/cli/tsc/99_main_compiler.js +++ b/cli/tsc/99_main_compiler.js @@ -525,7 +525,7 @@ delete Object.prototype.__proto__; if (logDebug) { debug(`host.readFile("${specifier}")`); } - return ops.op_load({ specifier }).data; + return ops.op_load(specifier).data; }, getCancellationToken() { // createLanguageService will call this immediately and cache it @@ -555,9 +555,7 @@ delete Object.prototype.__proto__; } /** @type {{ data: string; scriptKind: ts.ScriptKind; version: string; }} */ - const fileInfo = ops.op_load( - { specifier }, - ); + const fileInfo = ops.op_load(specifier); if (!fileInfo) { return undefined; } diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index 22c29a228e..0c3a5602ee 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -414,12 +414,6 @@ fn op_emit(state: &mut OpState, #[serde] args: EmitArgs) -> bool { true } -#[derive(Debug, Deserialize)] -struct LoadArgs { - /// The fully qualified specifier that should be loaded. - specifier: String, -} - pub fn as_ts_script_kind(media_type: MediaType) -> i32 { match media_type { MediaType::JavaScript => 1, @@ -453,36 +447,37 @@ struct LoadResponse { #[serde] fn op_load( state: &mut OpState, - #[serde] v: LoadArgs, + #[string] load_specifier: &str, ) -> Result { let state = state.borrow_mut::(); - let specifier = normalize_specifier(&v.specifier, &state.current_dir) + let specifier = normalize_specifier(load_specifier, &state.current_dir) .context("Error converting a string module specifier for \"op_load\".")?; let mut hash: Option = None; let mut media_type = MediaType::Unknown; let graph = &state.graph; - let data = if &v.specifier == "internal:///.tsbuildinfo" { + let data = if load_specifier == "internal:///.tsbuildinfo" { state.maybe_tsbuildinfo.as_deref().map(Cow::Borrowed) // in certain situations we return a "blank" module to tsc and we need to // handle the request for that module here. - } else if &v.specifier == "internal:///missing_dependency.d.ts" { + } else if load_specifier == "internal:///missing_dependency.d.ts" { hash = Some("1".to_string()); media_type = MediaType::Dts; Some(Cow::Borrowed("declare const __: any;\nexport = __;\n")) - } else if let Some(name) = v.specifier.strip_prefix("asset:///") { + } else if let Some(name) = load_specifier.strip_prefix("asset:///") { let maybe_source = get_lazily_loaded_asset(name); hash = get_maybe_hash(maybe_source, state.hash_data); - media_type = MediaType::from_str(&v.specifier); + media_type = MediaType::from_str(load_specifier); maybe_source.map(Cow::Borrowed) } else { let specifier = if let Some(remapped_specifier) = - state.remapped_specifiers.get(&v.specifier) + state.remapped_specifiers.get(load_specifier) { remapped_specifier - } else if let Some(remapped_specifier) = state.root_map.get(&v.specifier) { + } else if let Some(remapped_specifier) = state.root_map.get(load_specifier) + { remapped_specifier } else { &specifier @@ -1062,13 +1057,8 @@ mod tests { Some("some content".to_string()), ) .await; - let actual = op_load::call( - &mut state, - LoadArgs { - specifier: "https://deno.land/x/mod.ts".to_string(), - }, - ) - .unwrap(); + let actual = + op_load::call(&mut state, "https://deno.land/x/mod.ts").unwrap(); assert_eq!( serde_json::to_value(actual).unwrap(), json!({ @@ -1087,13 +1077,8 @@ mod tests { Some("some content".to_string()), ) .await; - let actual = op_load::call( - &mut state, - LoadArgs { - specifier: "asset:///lib.dom.d.ts".to_string(), - }, - ) - .expect("should have invoked op"); + let actual = op_load::call(&mut state, "asset:///lib.dom.d.ts") + .expect("should have invoked op"); let expected = get_lazily_loaded_asset("lib.dom.d.ts").unwrap(); assert_eq!(actual.data.unwrap(), expected); assert!(actual.version.is_some()); @@ -1108,13 +1093,8 @@ mod tests { Some("some content".to_string()), ) .await; - let actual = op_load::call( - &mut state, - LoadArgs { - specifier: "internal:///.tsbuildinfo".to_string(), - }, - ) - .expect("should have invoked op"); + let actual = op_load::call(&mut state, "internal:///.tsbuildinfo") + .expect("should have invoked op"); assert_eq!( serde_json::to_value(actual).unwrap(), json!({ @@ -1128,13 +1108,8 @@ mod tests { #[tokio::test] async fn test_load_missing_specifier() { let mut state = setup(None, None, None).await; - let actual = op_load::call( - &mut state, - LoadArgs { - specifier: "https://deno.land/x/mod.ts".to_string(), - }, - ) - .expect("should have invoked op"); + let actual = op_load::call(&mut state, "https://deno.land/x/mod.ts") + .expect("should have invoked op"); assert_eq!( serde_json::to_value(actual).unwrap(), json!({