From 7bf267c197fc6a5cbafa0e72992b1cc7629ff989 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Mon, 11 Dec 2023 16:59:09 +0100
Subject: [PATCH] perf(lsp): simplify some of the startup code (#21538)

Remove some dead code in "99_main_compiler.js". Eagerly start the LSP
TSC host, it was adding some not needed complexity around the TSC thread code.
---
 cli/lsp/tsc.rs              | 12 +++++-------
 cli/tsc/99_main_compiler.js | 19 +++++--------------
 cli/tsc/mod.rs              | 10 +++++-----
 3 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs
index b4f4d52c76..594ef02a24 100644
--- a/cli/lsp/tsc.rs
+++ b/cli/lsp/tsc.rs
@@ -226,13 +226,9 @@ impl TsServer {
 
       let runtime = create_basic_runtime();
       runtime.block_on(async {
-        let mut started = false;
+        start_tsc(&mut ts_runtime, false).unwrap();
+
         while let Some((req, state_snapshot, tx, token)) = rx.recv().await {
-          if !started {
-            // TODO(@kitsonk) need to reflect the debug state of the lsp here
-            start(&mut ts_runtime, false).unwrap();
-            started = true;
-          }
           let value =
             request(&mut ts_runtime, state_snapshot, req, token.clone());
           let was_sent = tx.send(value).is_ok();
@@ -3778,6 +3774,8 @@ impl TscSpecifierMap {
   }
 }
 
+// TODO(bartlomieju): we have similar struct in `cli/tsc/mod.rs` - maybe at least change
+// the name of the struct to avoid confusion?
 struct State {
   last_id: usize,
   performance: Arc<Performance>,
@@ -4057,7 +4055,7 @@ deno_core::extension!(deno_tsc,
 
 /// Instruct a language server runtime to start the language server and provide
 /// it with a minimal bootstrap configuration.
-fn start(runtime: &mut JsRuntime, debug: bool) -> Result<(), AnyError> {
+fn start_tsc(runtime: &mut JsRuntime, debug: bool) -> Result<(), AnyError> {
   let init_config = json!({ "debug": debug });
   let init_src = format!("globalThis.serverInit({init_config});");
 
diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js
index 8f76425c15..d795e542b9 100644
--- a/cli/tsc/99_main_compiler.js
+++ b/cli/tsc/99_main_compiler.js
@@ -519,6 +519,7 @@ delete Object.prototype.__proto__;
       if (logDebug) {
         debug(`host.fileExists("${specifier}")`);
       }
+      // TODO(bartlomieju): is this assumption still valid?
       // this is used by typescript to find the libs path
       // so we can completely ignore it
       return false;
@@ -963,6 +964,9 @@ delete Object.prototype.__proto__;
    * @param {number} id
    * @param {any} data
    */
+  // TODO(bartlomieju): this feels needlessly generic, both type chcking
+  // and language server use it with inefficient serialization. Id is not used
+  // anyway...
   function respond(id, data = null) {
     ops.op_respond({ id, data });
   }
@@ -1046,6 +1050,7 @@ delete Object.prototype.__proto__;
     }
   }
 
+  let hasStarted = false;
   /** @param {{ debug: boolean; }} init */
   function serverInit({ debug: debugFlag }) {
     if (hasStarted) {
@@ -1063,19 +1068,6 @@ delete Object.prototype.__proto__;
     debug("serverRestart()");
   }
 
-  let hasStarted = false;
-
-  /** Startup the runtime environment, setting various flags.
-   * @param {{ debugFlag?: boolean; legacyFlag?: boolean; }} msg
-   */
-  function startup({ debugFlag = false }) {
-    if (hasStarted) {
-      throw new Error("The compiler runtime already started.");
-    }
-    hasStarted = true;
-    setLogDebug(!!debugFlag, "TS");
-  }
-
   // A build time only op that provides some setup information that is used to
   // ensure the snapshot is setup properly.
   /** @type {{ buildSpecifier: string; libs: string[]; nodeBuiltInModuleNames: string[] }} */
@@ -1161,7 +1153,6 @@ delete Object.prototype.__proto__;
   // checking TypeScript.
   /** @type {any} */
   const global = globalThis;
-  global.startup = startup;
   global.exec = exec;
   global.getAssets = getAssets;
 
diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs
index 2a820a6eea..6a239c1e90 100644
--- a/cli/tsc/mod.rs
+++ b/cli/tsc/mod.rs
@@ -326,6 +326,8 @@ pub struct Response {
   pub stats: Stats,
 }
 
+// TODO(bartlomieju): we have similar struct in `tsc.rs` - maybe at least change
+// the name of the struct to avoid confusion?
 #[derive(Debug)]
 struct State {
   hash_data: u64,
@@ -764,6 +766,8 @@ struct RespondArgs {
   pub stats: Stats,
 }
 
+// TODO(bartlomieju): this mechanism is questionable.
+// Can't we use something more efficient here?
 #[op2]
 fn op_respond(state: &mut OpState, #[serde] args: RespondArgs) {
   let state = state.borrow_mut::<State>();
@@ -822,7 +826,6 @@ pub fn exec(request: Request) -> Result<Response, AnyError> {
     },
   );
 
-  let startup_source = ascii_str!("globalThis.startup({ legacyFlag: false })");
   let request_value = json!({
     "config": request.config,
     "debug": request.debug,
@@ -841,9 +844,6 @@ pub fn exec(request: Request) -> Result<Response, AnyError> {
     ..Default::default()
   });
 
-  runtime
-    .execute_script(located_script_name!(), startup_source)
-    .context("Could not properly start the compiler runtime.")?;
   runtime.execute_script(located_script_name!(), exec_source)?;
 
   let op_state = runtime.op_state();
@@ -1007,7 +1007,7 @@ mod tests {
       .execute_script_static(
         "<anon>",
         r#"
-      if (!(startup)) {
+      if (!(globalThis.exec)) {
           throw Error("bad");
         }
         console.log(`ts version: ${ts.version}`);