From 82b2037f6e3b9423345cedcceb3ec0a721afa590 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 6 Jun 2023 17:07:46 -0400 Subject: [PATCH] perf(cli): conditionally load typescript declaration files (#19392) Closes #18583 --- cli/factory.rs | 16 ++++++++- cli/graph_util.rs | 36 +++++++++++++++---- cli/lsp/language_server.rs | 3 +- cli/tests/integration/bench_tests.rs | 16 +++++++++ cli/tests/integration/cache_tests.rs | 10 ++++++ cli/tests/integration/run_tests.rs | 20 ++++++++--- cli/tests/integration/test_tests.rs | 16 +++++++++ .../run/type_directives_js_main.js.out | 3 -- cli/tools/bench.rs | 34 +++++++----------- cli/tools/compile.rs | 14 +++++++- cli/tools/doc.rs | 5 +-- cli/tools/info.rs | 3 +- cli/tools/test.rs | 34 +++++++----------- cli/tools/vendor/mod.rs | 5 ++- cli/tools/vendor/test.rs | 3 +- cli/tsc/mod.rs | 5 +-- 16 files changed, 156 insertions(+), 67 deletions(-) delete mode 100644 cli/tests/testdata/run/type_directives_js_main.js.out diff --git a/cli/factory.rs b/cli/factory.rs index 17d141be14..78aefe7804 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -8,6 +8,7 @@ use crate::args::Lockfile; use crate::args::PackageJsonDepsProvider; use crate::args::StorageKeyResolver; use crate::args::TsConfigType; +use crate::args::TypeCheckMode; use crate::cache::Caches; use crate::cache::DenoDir; use crate::cache::DenoDirProvider; @@ -47,6 +48,7 @@ use crate::worker::HasNodeSpecifierChecker; use deno_core::error::AnyError; use deno_core::parking_lot::Mutex; +use deno_graph::GraphKind; use deno_runtime::deno_fs; use deno_runtime::deno_node::analyze::NodeCodeTranslator; use deno_runtime::deno_node::NodeResolver; @@ -537,7 +539,19 @@ impl CliFactory { } pub fn graph_container(&self) -> &Arc { - self.services.graph_container.get_or_init(Default::default) + self.services.graph_container.get_or_init(|| { + let graph_kind = match self.options.sub_command() { + DenoSubcommand::Cache(_) => GraphKind::All, + _ => { + if self.options.type_check_mode() == TypeCheckMode::None { + GraphKind::CodeOnly + } else { + GraphKind::All + } + } + }; + Arc::new(ModuleGraphContainer::new(graph_kind)) + }) } pub fn maybe_inspector_server(&self) -> &Option> { diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 976c2aeca5..55052b9d0b 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -23,6 +23,7 @@ use deno_core::ModuleSpecifier; use deno_core::TaskQueue; use deno_core::TaskQueuePermit; use deno_graph::source::Loader; +use deno_graph::GraphKind; use deno_graph::Module; use deno_graph::ModuleError; use deno_graph::ModuleGraph; @@ -200,6 +201,7 @@ impl ModuleGraphBuilder { pub async fn create_graph_with_loader( &self, + graph_kind: GraphKind, roots: Vec, loader: &mut dyn Loader, ) -> Result { @@ -210,7 +212,7 @@ impl ModuleGraphBuilder { let graph_npm_resolver = cli_resolver.as_graph_npm_resolver(); let analyzer = self.parsed_source_cache.as_analyzer(); - let mut graph = ModuleGraph::default(); + let mut graph = ModuleGraph::new(graph_kind); self .build_graph_with_npm_resolution( &mut graph, @@ -249,7 +251,13 @@ impl ModuleGraphBuilder { let graph_resolver = cli_resolver.as_graph_resolver(); let graph_npm_resolver = cli_resolver.as_graph_npm_resolver(); let analyzer = self.parsed_source_cache.as_analyzer(); - let mut graph = ModuleGraph::default(); + let should_type_check = + self.options.type_check_mode() != TypeCheckMode::None; + let graph_kind = match should_type_check { + true => GraphKind::All, + false => GraphKind::CodeOnly, + }; + let mut graph = ModuleGraph::new(graph_kind); self .build_graph_with_npm_resolution( &mut graph, @@ -272,7 +280,7 @@ impl ModuleGraphBuilder { graph_lock_or_exit(&graph, &mut lockfile.lock()); } - if self.options.type_check_mode() != TypeCheckMode::None { + if should_type_check { self .type_checker .check( @@ -338,10 +346,13 @@ impl ModuleGraphBuilder { pub async fn create_graph( &self, + graph_kind: GraphKind, roots: Vec, ) -> Result { let mut cache = self.create_graph_loader(); - self.create_graph_with_loader(roots, &mut cache).await + self + .create_graph_with_loader(graph_kind, roots, &mut cache) + .await } } @@ -404,15 +415,15 @@ fn get_resolution_error_bare_specifier( } } -#[derive(Default, Debug)] +#[derive(Debug)] struct GraphData { graph: Arc, checked_libs: HashMap>, } /// Holds the `ModuleGraph` and what parts of it are type checked. -#[derive(Default)] pub struct ModuleGraphContainer { + graph_kind: GraphKind, // Allow only one request to update the graph data at a time, // but allow other requests to read from it at any time even // while another request is updating the data. @@ -421,8 +432,19 @@ pub struct ModuleGraphContainer { } impl ModuleGraphContainer { + pub fn new(graph_kind: GraphKind) -> Self { + Self { + graph_kind, + update_queue: Default::default(), + graph_data: Arc::new(RwLock::new(GraphData { + graph: Arc::new(ModuleGraph::new(graph_kind)), + checked_libs: Default::default(), + })), + } + } + pub fn clear(&self) { - self.graph_data.write().graph = Default::default(); + self.graph_data.write().graph = Arc::new(ModuleGraph::new(self.graph_kind)); } /// Acquires a permit to modify the module graph without other code diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 7c4191c82d..66ad043ce9 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -11,6 +11,7 @@ use deno_core::serde_json::json; use deno_core::serde_json::Value; use deno_core::task::spawn; use deno_core::ModuleSpecifier; +use deno_graph::GraphKind; use deno_lockfile::Lockfile; use deno_npm::resolution::ValidSerializedNpmResolutionSnapshot; use deno_npm::NpmSystemInfo; @@ -273,7 +274,7 @@ impl LanguageServer { open_docs: &open_docs, }; let graph = module_graph_builder - .create_graph_with_loader(roots.clone(), &mut loader) + .create_graph_with_loader(GraphKind::All, roots.clone(), &mut loader) .await?; graph_util::graph_valid( &graph, diff --git a/cli/tests/integration/bench_tests.rs b/cli/tests/integration/bench_tests.rs index 5b7361b304..0fc2680765 100644 --- a/cli/tests/integration/bench_tests.rs +++ b/cli/tests/integration/bench_tests.rs @@ -3,6 +3,7 @@ use deno_core::url::Url; use test_util as util; use util::assert_contains; +use util::assert_not_contains; use util::env_vars_for_npm_tests; use util::TestContext; @@ -250,3 +251,18 @@ itest!(bench_no_lock { cwd: Some("lockfile/basic"), output: "lockfile/basic/bench.nolock.out", }); + +#[test] +fn conditionally_loads_type_graph() { + let context = TestContext::default(); + let output = context + .new_command() + .args("bench --reload -L debug run/type_directives_js_main.js") + .run(); + output.assert_matches_text("[WILDCARD] - FileFetcher::fetch() - specifier: file:///[WILDCARD]/subdir/type_reference.d.ts[WILDCARD]"); + let output = context + .new_command() + .args("bench --reload -L debug --no-check run/type_directives_js_main.js") + .run(); + assert_not_contains!(output.combined_output(), "type_reference.d.ts"); +} diff --git a/cli/tests/integration/cache_tests.rs b/cli/tests/integration/cache_tests.rs index 7975cbf193..e8449ca05e 100644 --- a/cli/tests/integration/cache_tests.rs +++ b/cli/tests/integration/cache_tests.rs @@ -1,6 +1,7 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use test_util::env_vars_for_npm_tests; +use test_util::TestContext; use test_util::TestContextBuilder; itest!(_036_import_map_fetch { @@ -181,3 +182,12 @@ fn cache_put_overwrite() { output.assert_matches_text("res1\n"); output.assert_exit_code(0); } + +#[test] +fn loads_type_graph() { + let output = TestContext::default() + .new_command() + .args("cache --reload -L debug run/type_directives_js_main.js") + .run(); + output.assert_matches_text("[WILDCARD] - FileFetcher::fetch() - specifier: file:///[WILDCARD]/subdir/type_reference.d.ts[WILDCARD]"); +} diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 31b541e1c5..2accd54445 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -12,6 +12,7 @@ use test_util::TempDir; use trust_dns_client::serialize::txt::Lexer; use trust_dns_client::serialize::txt::Parser; use util::assert_contains; +use util::assert_not_contains; use util::env_vars_for_npm_tests_no_sync_download; use util::TestContext; use util::TestContextBuilder; @@ -1277,11 +1278,20 @@ itest!(type_directives_02 { output: "run/type_directives_02.ts.out", }); -itest!(type_directives_js_main { - args: "run --reload -L debug run/type_directives_js_main.js", - output: "run/type_directives_js_main.js.out", - exit_code: 0, -}); +#[test] +fn type_directives_js_main() { + let context = TestContext::default(); + let output = context + .new_command() + .args("run --reload -L debug --check run/type_directives_js_main.js") + .run(); + output.assert_matches_text("[WILDCARD] - FileFetcher::fetch() - specifier: file:///[WILDCARD]/subdir/type_reference.d.ts[WILDCARD]"); + let output = context + .new_command() + .args("run --reload -L debug run/type_directives_js_main.js") + .run(); + assert_not_contains!(output.combined_output(), "type_reference.d.ts"); +} itest!(type_directives_redirect { args: "run --reload --check run/type_directives_redirect.ts", diff --git a/cli/tests/integration/test_tests.rs b/cli/tests/integration/test_tests.rs index 4dd29528fd..cbaea36bd1 100644 --- a/cli/tests/integration/test_tests.rs +++ b/cli/tests/integration/test_tests.rs @@ -3,6 +3,7 @@ use deno_core::url::Url; use test_util as util; use util::assert_contains; +use util::assert_not_contains; use util::env_vars_for_npm_tests; use util::wildcard_match; use util::TestContext; @@ -566,3 +567,18 @@ fn test_with_glob_config_and_flags() { assert_contains!(output, "glob/data/test1.js"); assert_contains!(output, "glob/data/test1.ts"); } + +#[test] +fn conditionally_loads_type_graph() { + let context = TestContext::default(); + let output = context + .new_command() + .args("test --reload -L debug run/type_directives_js_main.js") + .run(); + output.assert_matches_text("[WILDCARD] - FileFetcher::fetch() - specifier: file:///[WILDCARD]/subdir/type_reference.d.ts[WILDCARD]"); + let output = context + .new_command() + .args("test --reload -L debug --no-check run/type_directives_js_main.js") + .run(); + assert_not_contains!(output.combined_output(), "type_reference.d.ts"); +} diff --git a/cli/tests/testdata/run/type_directives_js_main.js.out b/cli/tests/testdata/run/type_directives_js_main.js.out deleted file mode 100644 index 7bca837f02..0000000000 --- a/cli/tests/testdata/run/type_directives_js_main.js.out +++ /dev/null @@ -1,3 +0,0 @@ -[WILDCARD] -DEBUG RS - [WILDCARD] - FileFetcher::fetch() - specifier: file:///[WILDCARD]/subdir/type_reference.d.ts -[WILDCARD] diff --git a/cli/tools/bench.rs b/cli/tools/bench.rs index 107fd2b9b0..1a5df92bf0 100644 --- a/cli/tools/bench.rs +++ b/cli/tools/bench.rs @@ -31,6 +31,7 @@ use deno_core::task::spawn; use deno_core::task::spawn_blocking; use deno_core::v8; use deno_core::ModuleSpecifier; +use deno_graph::GraphKind; use deno_runtime::permissions::Permissions; use deno_runtime::permissions::PermissionsContainer; use deno_runtime::tokio_util::create_and_run_current_thread; @@ -693,7 +694,11 @@ pub async fn run_benchmarks_with_watch( // file would have impact on other files, which is undesirable. let permissions = Permissions::from_options(&cli_options.permissions_options())?; - let no_check = cli_options.type_check_mode() == TypeCheckMode::None; + let type_check = cli_options.type_check_mode() != TypeCheckMode::None; + let graph_kind = match type_check { + true => GraphKind::All, + false => GraphKind::CodeOnly, + }; let resolver = |changed: Option>| { let paths_to_watch = bench_options.files.include.clone(); @@ -714,7 +719,7 @@ pub async fn run_benchmarks_with_watch( bench_modules.clone() }; let graph = module_graph_builder - .create_graph(bench_modules.clone()) + .create_graph(graph_kind, bench_modules.clone()) .await?; graph_valid_with_cli_options(&graph, &bench_modules, &cli_options)?; @@ -726,32 +731,19 @@ pub async fn run_benchmarks_with_watch( // This needs to be accessible to skip getting dependencies if they're already there, // otherwise this will cause a stack overflow with circular dependencies output: &mut HashSet<&'a ModuleSpecifier>, - no_check: bool, ) { if let Some(module) = maybe_module.and_then(|m| m.esm()) { for dep in module.dependencies.values() { if let Some(specifier) = &dep.get_code() { if !output.contains(specifier) { output.insert(specifier); - get_dependencies( - graph, - graph.get(specifier), - output, - no_check, - ); + get_dependencies(graph, graph.get(specifier), output); } } - if !no_check { - if let Some(specifier) = &dep.get_type() { - if !output.contains(specifier) { - output.insert(specifier); - get_dependencies( - graph, - graph.get(specifier), - output, - no_check, - ); - } + if let Some(specifier) = &dep.get_type() { + if !output.contains(specifier) { + output.insert(specifier); + get_dependencies(graph, graph.get(specifier), output); } } } @@ -761,7 +753,7 @@ pub async fn run_benchmarks_with_watch( // This bench module and all it's dependencies let mut modules = HashSet::new(); modules.insert(&specifier); - get_dependencies(&graph, graph.get(&specifier), &mut modules, no_check); + get_dependencies(&graph, graph.get(&specifier), &mut modules); paths_to_watch.extend( modules diff --git a/cli/tools/compile.rs b/cli/tools/compile.rs index 2ce03e3534..540c23fc86 100644 --- a/cli/tools/compile.rs +++ b/cli/tools/compile.rs @@ -2,6 +2,7 @@ use crate::args::CompileFlags; use crate::args::Flags; +use crate::args::TypeCheckMode; use crate::factory::CliFactory; use crate::standalone::is_standalone_binary; use crate::util::path::path_has_trailing_slash; @@ -10,6 +11,7 @@ use deno_core::anyhow::Context; use deno_core::error::generic_error; use deno_core::error::AnyError; use deno_core::resolve_url_or_path; +use deno_graph::GraphKind; use deno_runtime::colors; use std::path::Path; use std::path::PathBuf; @@ -44,10 +46,20 @@ pub async fn compile( let graph = Arc::try_unwrap( module_graph_builder - .create_graph_and_maybe_check(module_roots) + .create_graph_and_maybe_check(module_roots.clone()) .await?, ) .unwrap(); + let graph = if cli_options.type_check_mode() == TypeCheckMode::None { + graph + } else { + // In this case, the previous graph creation did type checking, which will + // create a module graph with types information in it. We don't want to + // store that in the eszip so create a code only module graph from scratch. + module_graph_builder + .create_graph(GraphKind::CodeOnly, module_roots) + .await? + }; let parser = parsed_source_cache.as_capturing_parser(); let eszip = eszip::EszipV2::from_graph(graph, &parser, Default::default())?; diff --git a/cli/tools/doc.rs b/cli/tools/doc.rs index 2cb53cb6ab..87fa253151 100644 --- a/cli/tools/doc.rs +++ b/cli/tools/doc.rs @@ -16,6 +16,7 @@ use deno_core::error::AnyError; use deno_core::resolve_path; use deno_core::resolve_url_or_path; use deno_doc as doc; +use deno_graph::GraphKind; use deno_graph::ModuleSpecifier; use std::path::PathBuf; @@ -43,7 +44,7 @@ pub async fn print_docs( Vec::new(), ); let analyzer = deno_graph::CapturingModuleAnalyzer::default(); - let mut graph = deno_graph::ModuleGraph::default(); + let mut graph = deno_graph::ModuleGraph::new(GraphKind::TypesOnly); graph .build( vec![source_file_specifier.clone()], @@ -87,7 +88,7 @@ pub async fn print_docs( file_fetcher.insert_cached(root); let graph = module_graph_builder - .create_graph(vec![root_specifier.clone()]) + .create_graph(GraphKind::TypesOnly, vec![root_specifier.clone()]) .await?; if let Some(lockfile) = maybe_lockfile { diff --git a/cli/tools/info.rs b/cli/tools/info.rs index 63a755369c..95a7da7b0f 100644 --- a/cli/tools/info.rs +++ b/cli/tools/info.rs @@ -11,6 +11,7 @@ use deno_core::resolve_url_or_path; use deno_core::serde_json; use deno_core::serde_json::json; use deno_graph::Dependency; +use deno_graph::GraphKind; use deno_graph::Module; use deno_graph::ModuleError; use deno_graph::ModuleGraph; @@ -43,7 +44,7 @@ pub async fn info(flags: Flags, info_flags: InfoFlags) -> Result<(), AnyError> { let mut loader = module_graph_builder.create_graph_loader(); loader.enable_loading_cache_info(); // for displaying the cache information let graph = module_graph_builder - .create_graph_with_loader(vec![specifier], &mut loader) + .create_graph_with_loader(GraphKind::All, vec![specifier], &mut loader) .await?; if let Some(lockfile) = maybe_lockfile { diff --git a/cli/tools/test.rs b/cli/tools/test.rs index f78e325394..bc8f685999 100644 --- a/cli/tools/test.rs +++ b/cli/tools/test.rs @@ -39,6 +39,7 @@ use deno_core::task::spawn_blocking; use deno_core::url::Url; use deno_core::v8; use deno_core::ModuleSpecifier; +use deno_graph::GraphKind; use deno_runtime::deno_io::Stdio; use deno_runtime::deno_io::StdioPipe; use deno_runtime::fmt_errors::format_js_error; @@ -1706,7 +1707,11 @@ pub async fn run_tests_with_watch( // file would have impact on other files, which is undesirable. let permissions = Permissions::from_options(&cli_options.permissions_options())?; - let no_check = cli_options.type_check_mode() == TypeCheckMode::None; + let type_check = cli_options.type_check_mode() != TypeCheckMode::None; + let graph_kind = match type_check { + true => GraphKind::All, + false => GraphKind::CodeOnly, + }; let log_level = cli_options.log_level(); let resolver = |changed: Option>| { @@ -1731,7 +1736,7 @@ pub async fn run_tests_with_watch( test_modules.clone() }; let graph = module_graph_builder - .create_graph(test_modules.clone()) + .create_graph(graph_kind, test_modules.clone()) .await?; graph_valid_with_cli_options(&graph, &test_modules, &cli_options)?; @@ -1743,32 +1748,19 @@ pub async fn run_tests_with_watch( // This needs to be accessible to skip getting dependencies if they're already there, // otherwise this will cause a stack overflow with circular dependencies output: &mut HashSet<&'a ModuleSpecifier>, - no_check: bool, ) { if let Some(module) = maybe_module.and_then(|m| m.esm()) { for dep in module.dependencies.values() { if let Some(specifier) = &dep.get_code() { if !output.contains(specifier) { output.insert(specifier); - get_dependencies( - graph, - graph.get(specifier), - output, - no_check, - ); + get_dependencies(graph, graph.get(specifier), output); } } - if !no_check { - if let Some(specifier) = &dep.get_type() { - if !output.contains(specifier) { - output.insert(specifier); - get_dependencies( - graph, - graph.get(specifier), - output, - no_check, - ); - } + if let Some(specifier) = &dep.get_type() { + if !output.contains(specifier) { + output.insert(specifier); + get_dependencies(graph, graph.get(specifier), output); } } } @@ -1778,7 +1770,7 @@ pub async fn run_tests_with_watch( // This test module and all it's dependencies let mut modules = HashSet::new(); modules.insert(&specifier); - get_dependencies(&graph, graph.get(&specifier), &mut modules, no_check); + get_dependencies(&graph, graph.get(&specifier), &mut modules); paths_to_watch.extend( modules diff --git a/cli/tools/vendor/mod.rs b/cli/tools/vendor/mod.rs index 5690f5b227..61ada605c5 100644 --- a/cli/tools/vendor/mod.rs +++ b/cli/tools/vendor/mod.rs @@ -10,6 +10,7 @@ use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::resolve_url_or_path; +use deno_graph::GraphKind; use log::warn; use crate::args::CliOptions; @@ -371,7 +372,9 @@ async fn create_graph( .map(|p| resolve_url_or_path(p, initial_cwd)) .collect::, _>>()?; - module_graph_builder.create_graph(entry_points).await + module_graph_builder + .create_graph(GraphKind::All, entry_points) + .await } #[cfg(test)] diff --git a/cli/tools/vendor/test.rs b/cli/tools/vendor/test.rs index e8a474ed34..08b6d8355b 100644 --- a/cli/tools/vendor/test.rs +++ b/cli/tools/vendor/test.rs @@ -16,6 +16,7 @@ use deno_core::serde_json; use deno_graph::source::LoadFuture; use deno_graph::source::LoadResponse; use deno_graph::source::Loader; +use deno_graph::GraphKind; use deno_graph::ModuleGraph; use import_map::ImportMap; @@ -279,7 +280,7 @@ async fn build_test_graph( Default::default(), ) }); - let mut graph = ModuleGraph::default(); + let mut graph = ModuleGraph::new(GraphKind::All); graph .build( roots, diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index d9f9b8b531..83fd84f9dc 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -839,6 +839,7 @@ mod tests { use crate::args::TsConfig; use deno_core::futures::future; use deno_core::OpState; + use deno_graph::GraphKind; use deno_graph::ModuleGraph; use std::fs; @@ -882,7 +883,7 @@ mod tests { let hash_data = maybe_hash_data.unwrap_or(0); let fixtures = test_util::testdata_path().join("tsc2"); let mut loader = MockLoader { fixtures }; - let mut graph = ModuleGraph::default(); + let mut graph = ModuleGraph::new(GraphKind::TypesOnly); graph .build(vec![specifier], &mut loader, Default::default()) .await; @@ -908,7 +909,7 @@ mod tests { let hash_data = 123; // something random let fixtures = test_util::testdata_path().join("tsc2"); let mut loader = MockLoader { fixtures }; - let mut graph = ModuleGraph::default(); + let mut graph = ModuleGraph::new(GraphKind::TypesOnly); graph .build(vec![specifier.clone()], &mut loader, Default::default()) .await;