Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: analyze cjs exports and emit typescript in parallel #23856

Merged
merged 6 commits into from
May 18, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented May 16, 2024

Two main things we can do in parallel:

  1. CJS export analysis
  2. TypeScript emit

This makes the loader async to make doing this easier.

Deleting the node analysis cache between runs and importing some npm packages (this only makes the first run much faster due to the caches... the second run is the same speed as befroe):

Benchmark 1: ../deno/deno_old run -A ts_morph.ts
  Time (mean ± σ):     406.1 ms ±   6.5 ms    [User: 358.1 ms, System: 62.2 ms]
  Range (min … max):   395.9 ms … 412.9 ms    10 runs
 
Benchmark 2: ../deno/target/release/deno run -A ts_morph.ts
  Time (mean ± σ):     278.2 ms ±   9.3 ms    [User: 411.1 ms, System: 81.4 ms]
  Range (min … max):   266.1 ms … 292.9 ms    10 runs
 
Summary
  ../deno/target/release/deno run -A ts_morph.ts ran
    1.46 ± 0.05 times faster than ../deno/deno_old run -A ts_morph.ts
% cat ts_morph.ts
import "npm:ts-morph@22.0.0";
import "npm:ts-morph@21.0.0";
import "npm:ts-morph@20.0.0";
import "npm:ts-morph@19.0.0";

Deleting the TypeScript emit and module analysis cache between runs and importing multiple copies of Dax:

Benchmark 1: ../deno/deno_old run -A dax_deno_land_x.ts
  Time (mean ± σ):     587.3 ms ±  24.0 ms    [User: 347.5 ms, System: 221.0 ms]
  Range (min … max):   569.2 ms … 650.7 ms    10 runs
 
Benchmark 2: ../deno/target/release/deno run -A dax_deno_land_x.ts
  Time (mean ± σ):     489.3 ms ±  14.3 ms    [User: 554.8 ms, System: 411.2 ms]
  Range (min … max):   473.9 ms … 510.2 ms    10 runs
 
Summary
  ../deno/target/release/deno run -A dax_deno_land_x.ts ran
    1.20 ± 0.06 times faster than ../deno/deno_old run -A dax_deno_land_x.ts
scratch % cat dax_deno_land_x.ts
import "https://deno.land/x/dax@0.39.2/mod.ts";
import "https://deno.land/x/dax@0.38.0/mod.ts";
import "https://deno.land/x/dax@0.37.0/mod.ts";
import "https://deno.land/x/dax@0.36.0/mod.ts";
import "https://deno.land/x/dax@0.35.0/mod.ts";

Note that this is still parsing typescript files sequentially because it doesn't include (denoland/deno_graph#477). It is just emitting typescript files in parallel.

@dsherret dsherret marked this pull request as ready for review May 17, 2024 00:06
@dsherret dsherret changed the title perf: make loader async in order to do more tasks in parallel perf: analyze cjs exports and emit typescript in parallel May 17, 2024
@@ -113,6 +117,7 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer> NodeCodeTranslator<TCjsCodeAnalyzer> {
reexports_to_handle.push_back((reexport, specifier.clone()));
}

// todo(dsherret): we could run this analysis concurrently in a FuturesOrdered
Copy link
Member Author

@dsherret dsherret May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a future perf improvement. Opened #23859

&self,
graph: &ModuleGraph,
) -> Result<(), AnyError> {
// todo(dsherret): we could do this concurrently
Copy link
Member Author

@dsherret dsherret May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another future perf improvement #23860

@dsherret dsherret requested a review from bartlomieju May 17, 2024 02:32
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}
}

fn load_prepared_module_sync(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this temporary or will we always need both sync and async versions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source map lookup is sync, so we'll probably need this unless we move the spawn blocking to a higher level (I investigated doing this just now, and the performance is equivalent, but it means that all our code in the worker needs to be Send/Sync, which is something I don't think we want to do)

Comment on lines +780 to +784
deno_core::ModuleLoadResponse::Async(
async move {
inner
.load_inner(
&specifier,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by fix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's async these now happen out of order sometimes.

@dsherret dsherret merged commit a2dbcf9 into denoland:main May 18, 2024
17 checks passed
@dsherret dsherret deleted the perf_async_loader branch May 18, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants