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(remix-dev/vite): extract route module exports in parallel #8111

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 23, 2023

This is a first idea extracted from #8102.
I think currently "for loop + await" in getDevManifest could be a bottleneck of initial server startup time for a project with many routes. Assuming vite can genuinely run transform in parallel (multi threaded in esbuild? or at least some file IO?), wrapping multiple getRouteModuleExports calls by Promise.all seems to help.

My example is a small project (the code is here hi-ogawa/ytsub-v3#519) but I can still observe some difference. It's a primitive benchmark, but here is a quick comparison (I'm adding console.time/timeEnd to check the timing of code blocks):

before
    console.time("getDevManifest");
    for (let [key, route] of Object.entries(pluginConfig.routes)) {
      console.time("getDevManifest: " + key);
      let sourceExports = await getRouteModuleExports(viteChildCompiler, pluginConfig, route.file)
      console.timeEnd("getDevManifest: " + key);
      routes[key] = ...
    }
    console.timeEnd("getDevManifest");
getDevManifest: root: 746.57ms
getDevManifest: routes/share-target.server: 161.661ms
getDevManifest: routes/caption-editor: 61.558ms
getDevManifest: routes/index.server: 375.663ms
getDevManifest: routes/share-target: 70.679ms
getDevManifest: routes/bookmarks: 34.779ms
getDevManifest: routes/videos: 19.5ms
getDevManifest: routes/decks: 13.154ms
getDevManifest: routes/index: 23.105ms
getDevManifest: routes/bookmarks/history-chart.server: 59.469ms
getDevManifest: routes/caption-editor/watch.server: 17.444ms
getDevManifest: routes/users/password-new.server: 13.074ms
getDevManifest: routes/bookmarks/history-chart: 31.983ms
getDevManifest: routes/decks/$id/_utils.server: 170.949ms
getDevManifest: routes/decks/$id/export.server: 42.11ms
getDevManifest: routes/decks/$id/history-graph: 131.047ms
getDevManifest: routes/bookmarks/index.server: 91.59ms
getDevManifest: routes/decks/$id/index.server: 22.335ms
getDevManifest: routes/users/register.server: 30.534ms
getDevManifest: routes/caption-editor/index: 48.431ms
getDevManifest: routes/caption-editor/watch: 15.765ms
getDevManifest: routes/users/password-reset: 11.639ms
getDevManifest: routes/dev/health-check-db: 44.698ms
getDevManifest: routes/users/verify.server: 15.283ms
getDevManifest: routes/videos/index.server: 48.423ms
getDevManifest: routes/decks/$id/practice: 46.263ms
getDevManifest: routes/decks/index.server: 32.679ms
getDevManifest: routes/users/password-new: 15.539ms
getDevManifest: routes/decks/$id/history: 60.683ms
getDevManifest: routes/videos/$id.server: 8.427ms
getDevManifest: routes/videos/new.server: 10.314ms
getDevManifest: routes/decks/$id/export: 4.444ms
getDevManifest: routes/decks/new.server: 7.291ms
getDevManifest: routes/dev/health-check: 9.03ms
getDevManifest: routes/bookmarks/index: 8.407ms
getDevManifest: routes/decks/$id/index: 7.91ms
getDevManifest: routes/users/me.server: 5.586ms
getDevManifest: routes/decks/$id/edit: 9.36ms
getDevManifest: routes/users/register: 7.715ms
getDevManifest: routes/dev/debug-env: 8.5ms
getDevManifest: routes/decks/import: 5.532ms
getDevManifest: routes/users/signin: 6.436ms
getDevManifest: routes/users/verify: 4.601ms
getDevManifest: routes/videos/index: 10.412ms
getDevManifest: routes/decks/index: 7.851ms
getDevManifest: routes/dev/emails: 8.077ms
getDevManifest: routes/videos/$id: 14.25ms
getDevManifest: routes/videos/new: 10.413ms
getDevManifest: routes/decks/new: 7.233ms
getDevManifest: routes/dev/debug: 9.671ms
getDevManifest: routes/dev/knex: 4.937ms
getDevManifest: routes/dev/stop: 8.239ms
getDevManifest: routes/users/me: 6.347ms
getDevManifest: 2.654s
after
    console.time("getDevManifest");
    console.time("getRouteManifestModuleExports");
    let routeManifestExports = await getRouteManifestModuleExports(viteChildCompiler, pluginConfig);
    console.timeEnd("getRouteManifestModuleExports");
    for (let [key, route] of Object.entries(pluginConfig.routes)) {
      console.time("getDevManifest: " + key);
      let sourceExports = routeManifestExports[key];
      console.timeEnd("getDevManifest: " + key);
      routes[key] = ...
    }
    console.timeEnd("getDevManifest");
getRouteManifestModuleExports: 1.157s
getDevManifest: root: 0.003ms
getDevManifest: routes/share-target.server: 0.003ms
getDevManifest: routes/caption-editor: 0.004ms
getDevManifest: routes/index.server: 0.002ms
getDevManifest: routes/share-target: 0.002ms
getDevManifest: routes/bookmarks: 0.003ms
getDevManifest: routes/videos: 0.002ms
getDevManifest: routes/decks: 0.002ms
getDevManifest: routes/index: 0.003ms
getDevManifest: routes/bookmarks/history-chart.server: 0.005ms
getDevManifest: routes/caption-editor/watch.server: 0.003ms
getDevManifest: routes/users/password-new.server: 0.004ms
getDevManifest: routes/bookmarks/history-chart: 0.003ms
getDevManifest: routes/decks/$id/_utils.server: 0.003ms
getDevManifest: routes/decks/$id/export.server: 0.006ms
getDevManifest: routes/decks/$id/history-graph: 0.003ms
getDevManifest: routes/bookmarks/index.server: 0.002ms
getDevManifest: routes/decks/$id/index.server: 0.002ms
getDevManifest: routes/users/register.server: 0.002ms
getDevManifest: routes/caption-editor/index: 0.003ms
getDevManifest: routes/caption-editor/watch: 0.004ms
getDevManifest: routes/users/password-reset: 0.002ms
getDevManifest: routes/dev/health-check-db: 0.001ms
getDevManifest: routes/users/verify.server: 0.002ms
getDevManifest: routes/videos/index.server: 0.002ms
getDevManifest: routes/decks/$id/practice: 0.002ms
getDevManifest: routes/decks/index.server: 0.002ms
getDevManifest: routes/users/password-new: 0.001ms
getDevManifest: routes/decks/$id/history: 0.001ms
getDevManifest: routes/videos/$id.server: 0.002ms
getDevManifest: routes/videos/new.server: 0.001ms
getDevManifest: routes/decks/$id/export: 0.001ms
getDevManifest: routes/decks/new.server: 0.002ms
getDevManifest: routes/dev/health-check: 0.002ms
getDevManifest: routes/bookmarks/index: 0.002ms
getDevManifest: routes/decks/$id/index: 0.003ms
getDevManifest: routes/users/me.server: 0.001ms
getDevManifest: routes/decks/$id/edit: 0.001ms
getDevManifest: routes/users/register: 0.002ms
getDevManifest: routes/dev/debug-env: 0.001ms
getDevManifest: routes/decks/import: 0.001ms
getDevManifest: routes/users/signin: 0.002ms
getDevManifest: routes/users/verify: 0.002ms
getDevManifest: routes/videos/index: 0.002ms
getDevManifest: routes/decks/index: 0.003ms
getDevManifest: routes/dev/emails: 0.002ms
getDevManifest: routes/videos/$id: 0.001ms
getDevManifest: routes/videos/new: 0.003ms
getDevManifest: routes/decks/new: 0.002ms
getDevManifest: routes/dev/debug: 0.002ms
getDevManifest: routes/dev/knex: 0.003ms
getDevManifest: routes/dev/stop: 0.002ms
getDevManifest: routes/users/me: 0.002ms
getDevManifest: 1.160s

Copy link

changeset-bot bot commented Nov 23, 2023

⚠️ No Changeset found

Latest commit: 8c6645d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

opting to merge all perf improvements into a single changeset instead
Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@markdalgleish markdalgleish changed the title perf(remix-dev/vite): extract route module exports in parallel for route manifest generaion perf(remix-dev/vite): extract route module exports in parallel Nov 26, 2023
@markdalgleish markdalgleish merged commit 62cb5a1 into remix-run:dev Nov 26, 2023
@hi-ogawa hi-ogawa deleted the perf-vite-parallel-getRouteModuleExports-manifest branch November 26, 2023 23:18
Copy link
Contributor

github-actions bot commented Dec 5, 2023

🤖 Hello there,

We just published version 2.4.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.4.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants