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

Vite with Large Apps: Very slow initial request and slow subsequent requests #7868

Closed
1 task done
dmarkow opened this issue Nov 1, 2023 · 19 comments
Closed
1 task done

Comments

@dmarkow
Copy link
Contributor

dmarkow commented Nov 1, 2023

What version of Remix are you using?

2.2.0

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

Adding per a Discord discussion with @pcattori. Given a large app with many routes and components, the first page request after starting vite dev can result in hundreds of network requests. In my case most pages make around 250-300 requests, but another person in that same discussion had close to 600. In addition, every following request to any route (via navigation or actions/loaders) has a significant delay (2+ seconds per page for me). Including both of these issues together since they seem closely related in the way the plugin works.

Slow Initial Request

In my case, the first page request stalls for around 12 seconds while vite does its thing. After this, the app appears to load, but is not responsive for another 12 seconds while it runs 250 network requests for individual components and dependencies. This seems to be a common issue with Vite in dev mode due to the lack of bundling. But there may be some optimization opportunities on the remix plugin side.

I first ran vite dev and loaded a page, which let vite do it's initial new dependencies optimized... thing. I then restarted vite and ran vite dev --profile, made a browser request, then loaded the profile result into speedscope.app. Looking at it in Left Heavy mode, the time spent in resolveConfig and then flatRoutes is close to 9 seconds total. This is because resolveConfig is being called every time the plugin is loaded, which happens for each individual request. So even though it runs relatively quickly (around 30ms in my case), it's running hundreds of times and returning the same result every time. My initial thought was to cache the results of resolveConfig, but adding/renaming routes will break.

The next largest bottleneck in the remix plugin is at the transform function for remix-remix-react-proxy. For my initial page load, it's spending around 3-4 seconds here, most likely due to parsing the code of every single file in my project to see if it has "@remix-run/react" in it.

CleanShot 2023-11-01 at 16 49 30@2x

Slow Subsequent Requests

The transform bottleneck above is also causing subsequent requests to have a delay of 2+ seconds for me, which includes any navigation or calling any loaders/actions. This also slows down HMR -- even though vite almost instantly sends the updated component to the browser, rendering is paused while HDR re-runs the loader.

The plugin is set to invalidate the server build on every request, which causes all of the routes to be re-transformed. Since it's only re-transforming the routes and not every component, it is only taking 2 seconds instead of 3-4 like the initial page load, but it's enough to make the app feel very sluggish in development.

I tried commenting out the code that invalidates the server modules which makes the app very responsive after the initial load. But it has the same downside as caching resolveConfig: I now have to restart vite dev any time I add or rename routes. @pcattori mentioned figuring out a way to only invalidate when the changed files include routes or the config.

Expected Behavior

Reasonably quick development mode. Definitely not expecting the performance of a small 3-page app, but right now it's a significant slowdown from the old remix dev server (which took around 10 seconds for the initial build/page load).

Actual Behavior

25 second initial loads and sluggish navigation on following loads.

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 2, 2023

Spent some time this morning trying various caching ideas, but everything I tried broke something, usually related to adding/renaming routes. There are three key things that need to be cached or sped up in some way:

  1. remix-react-refresh-babel: The call to resolvePluginConfig() here is running hundreds of times on startup causing the slow initial page load.
  2. remix-remove-server-exports: Same, but to a lesser extent since it's not affecting the ssr build
  3. The middleware in configureServer(vite). The resolvePluginConfig() here is fine, it only runs once per route request and doesn't seem to run for every individual component the page loads. However, it's invalidating all of the server modules on every request which is adding the 2 second delay.

Fixing 1 and 2 seemed a little easier, but the second I tried to add caching around the invalidating, everything broke -- it seems there are situations where even if the resolvePluginConfig() results haven't changed, the virtual modules still need to be invalidated. I'll explore some more when I have time.

@mikekidder
Copy link

@dmarkow - I am going to be curious to see if "remix-react-refresh-babel" is even needed. Especially since Vite does not need Babel transforms.

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 2, 2023

@mikekidder Vite itself may not, but Remix does use a babel transform to 1) inject react-refresh code and 2) remove server exports (loader/action/headers) from route files.

@ZipBrandon
Copy link

@dmarkow Have you had success with vite.config.ts using optimizeDeps ? Would this help the initial load to the server by eagerly optimizing the bundles?

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 2, 2023

@ZipBrandon I haven't explored that yet. On the old dev server my initial build time was 10-11 seconds, so Vite being at 12 for the initial build in my case is roughly the same. The caching I'm testing out right now should address the second 12 seconds where it loads in 250+ (or in your case almost 600) modules slowly.

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 2, 2023

I think I figured out my pain points above. I ended up adding two caches. The first one is for the middleware, which checks and refreshes the cache on each request, and only invalidates the virtual modules if the config changed. This makes site navigation a ton more responsive in dev, and speeds up HDR quite a bit. The other cache is initially set in the plugin config and used anywhere there are excessive calls to resolvePluginCache(). This cache gets refreshed in remix-hmr-updates, which should be the first part of the plugin to run anytime there is a file change as far as I can tell.

So far, this caching has drastically reduced that 8.5 seconds spent in resolveConfig down to 146ms. My next bottleneck on the profile graph is replaceImportSpecifier being called for a bunch of files in remix-remix-react-proxy (3+ seconds). The slowness here is Babel's parse/traverse/generate which I'm not sure how we speed up without moving away from Babel completely. Maybe switching to something faster like SWC?

I have a patch with the caching, I'd like to use it locally for a little bit before I create a PR around this. All Vite integration tests are still passing. The only jankiness I'm seeing is that adding a new route while the server is running and then trying to navigate to it sometimes causes an error, but if I hard-reload the browser it comes up fine without having to restart the dev server. And I'm getting the same issue without my patch which may warrant a separate issue.

Here's the patch if anyone wants to test -- you can use patch-package and drop it in your project's patches folder. @ZipBrandon I'd be curious to see what kind of speed increases you see on initial build.

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 2, 2023

// Don't transform files that don't need the proxy
if (
  !code.includes("@remix-run/react") &&
  !code.includes("LiveReload")
) {
  return;
}

// Rewrite imports to use the proxy
return replaceImportSpecifier({
  code,
  specifier: "@remix-run/react",
  replaceWith: remixReactProxyId,
});

@pcattori Is there a reason the RefreshRuntime is being injected into every file that imports from @remix-run/react? Shouldn't it just be the root file? Making it just inject into the root route will mostly eliminate the Babel slowdown. I'm wondering if this should actually be:

if (!(code.includes("@remix-run/react") && code.includes("LiveReload")) {
  return;
}

@ZipBrandon
Copy link

@dmarkow I'm currently trying the patch out and I'll profile on my end to see what it looks like around resolveConfig. One thing that I'm hitting now is the first load gets a boatload of deps to optimize which impacts timing. I really want to resolve this. It doesn't seem that optimizeDeps is being honored -- or I am misconfiguring it out somehow.

GET /new/4a812d66-cfac-4d83-9096-54b868c0dba9/sales_page 200 - - 26761.090 ms
4:17:00 PM [vite] ✨ new dependencies optimized: i18next, i18next-browser-languagedetector, react-i18next, remix-i18next, @remix-run/react, @sentry/remix, immer, react-dom, remeda, remix-utils/client-only, is-ios, react-use/esm/index.js, clsx, tailwind-merge, react-router, react-scroll, react-fast-compare, react-router-dom, uuid, @reach/portal, remix-validated-form, schema-to-yup, tiny-invariant, zustand, yup, react-markdown, react-reverse-portal, framer-motion, react-merge-refs, react-popper-tooltip, @headlessui/react, @heroicons/react/24/outline, smooth-scroll-into-view-if-needed, lodash-es/isObject.js, lodash-es/merge.js, lodash-es/reduce.js, @sindresorhus/slugify, fecha, @remix-validated-form/with-yup, @remix-validated-form/with-zod, date-fns, lodash-es/get.js, react-waypoint, react-helmet, react-error-boundary, lodash-es/identity.js, lodash-es/isString.js, @heroicons/react/24/solid, @floating-ui/react, downshift, lodash-es/find.js, match-sorter, @react-aria/button, @react-aria/searchfield, @react-stately/searchfield, @udecode/plate-core, @tremor/react, @tanstack/react-virtual, date-fns/locale/es/index.js, date-fns/locale/fr/index.js, react-day-picker, xstate, lodash/merge.js, remix-routes, lodash-es/set.js, lodash-es, graphql-tag, @dnd-kit/core, @dnd-kit/sortable, nanoid, @udecode/plate, @udecode/plate-ui-mention, slate-react, remark-gfm, remark-parse, unified, lodash/get.js, react-idle-timer, ramda, use-resize-observer/polyfilled.js, @xstate/react, @dnd-kit/utilities, @udecode/plate-basic-marks, @udecode/plate-common, @udecode/plate-heading, @udecode/plate-list, dayjs, omit-deep-lodash, @udecode/slate, @udecode/slate-react, dayjs/plugin/customParseFormat, dayjs/plugin/weekday, dayjs/locale/af, dayjs/locale/am, dayjs/locale/ar-dz, dayjs/locale/ar-iq, dayjs/locale/ar-kw, dayjs/locale/ar-ly, dayjs/locale/ar-ma, dayjs/locale/ar-sa, dayjs/locale/ar-tn, dayjs/locale/ar, dayjs/locale/az, dayjs/locale/bg, dayjs/locale/bi, dayjs/locale/bm, dayjs/locale/bn-bd, dayjs/locale/bn, dayjs/locale/bo, dayjs/locale/br, dayjs/locale/ca, dayjs/locale/cs, dayjs/locale/cv, dayjs/locale/cy, dayjs/locale/da, dayjs/locale/de-at, dayjs/locale/de-ch, dayjs/locale/de, dayjs/locale/dv, dayjs/locale/el, dayjs/locale/en-au, dayjs/locale/en-gb, dayjs/locale/en-ie, dayjs/locale/en-il, dayjs/locale/en-in, dayjs/locale/en-nz, dayjs/locale/en-sg, dayjs/locale/en-tt, dayjs/locale/en, dayjs/locale/eo, dayjs/locale/es-do, dayjs/locale/es-mx, dayjs/locale/es-pr, dayjs/locale/es-us, dayjs/locale/es, dayjs/locale/et, dayjs/locale/eu, dayjs/locale/fa, dayjs/locale/fi, dayjs/locale/fo, dayjs/locale/fr-ch, dayjs/locale/fr, dayjs/locale/fy, dayjs/locale/ga, dayjs/locale/gd, dayjs/locale/gl, dayjs/locale/gom-latn, dayjs/locale/gu, dayjs/locale/he, dayjs/locale/hi, dayjs/locale/hr, dayjs/locale/ht, dayjs/locale/hu, dayjs/locale/hy-am, dayjs/locale/id, dayjs/locale/is, dayjs/locale/it-ch, dayjs/locale/it, dayjs/locale/ja, dayjs/locale/jv, dayjs/locale/ka, dayjs/locale/kk, dayjs/locale/ko, dayjs/locale/ku, dayjs/locale/ky, dayjs/locale/lb, dayjs/locale/lo, dayjs/locale/lt, dayjs/locale/lv, dayjs/locale/me, dayjs/locale/mi, dayjs/locale/mk, dayjs/locale/ml, dayjs/locale/mn, dayjs/locale/ms-my, dayjs/locale/ms, dayjs/locale/mt, dayjs/locale/my, dayjs/locale/nb, dayjs/locale/ne, dayjs/locale/nl-be, dayjs/locale/nl, dayjs/locale/nn, dayjs/locale/oc-lnc, dayjs/locale/pa-in, dayjs/locale/pl, dayjs/locale/pt-br, dayjs/locale/pt, dayjs/locale/rn, dayjs/locale/ro, dayjs/locale/ru, dayjs/locale/rw, dayjs/locale/sd, dayjs/locale/se, dayjs/locale/si, dayjs/locale/sk, dayjs/locale/sl, dayjs/locale/sq, dayjs/locale/sr, dayjs/locale/sr-cyrl, dayjs/locale/ss, dayjs/locale/sv-fi, dayjs/locale/sv, dayjs/locale/sw, dayjs/locale/ta, dayjs/locale/te, dayjs/locale/tg, dayjs/locale/th, dayjs/locale/tk, dayjs/locale/tl-ph, dayjs/locale/tlh, dayjs/locale/tr, dayjs/locale/tzl, dayjs/locale/tzm-latn, dayjs/locale/tzm, dayjs/locale/ug-cn, dayjs/locale/uk, dayjs/locale/ur, dayjs/locale/uz-latn, dayjs/locale/uz, dayjs/locale/vi, dayjs/locale/x-pseudo, dayjs/locale/yo, dayjs/locale/zh-cn, dayjs/locale/zh-hk, dayjs/locale/zh-tw, dayjs/locale/zh, dayjs/plugin/isBetween
4:17:00 PM [vite] ✨ optimized dependencies changed. reloading

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 2, 2023

@ZipBrandon I think that happens on the first build after changing anything in node_modules (which patch-package does). If you kill vite and start again, does it re-optimize those same dependencies again?

@ZipBrandon
Copy link

@dmarkow Improvement with your patch!
Unpatched
image

patched:
image

Yes, unfortunately if I kill vite and start again it re-optimizes it. Worse yet, if I navigate to a route that has some other dependencies that it discovers then it has to reoptimize those + all again.

@pcattori
Copy link
Contributor

pcattori commented Nov 2, 2023

@pcattori Is there a reason the RefreshRuntime is being injected into every file that imports from @remix-run/react? Shouldn't it just be the root file? Making it just inject into the root route will mostly eliminate the Babel slowdown. I'm wondering if this should actually be:

if (!(code.includes("@remix-run/react") && code.includes("LiveReload")) {
  return;
}

🤦 yep that looks like what we meant to write 😅 and this is why its unstable!

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 2, 2023

An added bonus is that fixing these things drastically improves builds. Before any patches, vite build && vite build --ssr was taking my app 129 seconds. After patches (including a fix for the RefreshRuntime thing above), it's down to 24 seconds.

@ZipBrandon
Copy link

@dmarkow I was running at ~5m 45s. Now 22s!

@markdalgleish
Copy link
Member

markdalgleish commented Nov 2, 2023

@dmarkow Thanks for picking that up! I've just opened a PR addressing this: #7883

@ZipBrandon
Copy link

@markdalgleish I have successfully been using @dmarkow's patch for the last 3 days. I just tried to go to nightly-20231103 where I see your change in it. My build times for prod are back to 5 minutes 45 seconds from ~22 seconds when I first patched it with @dmarkow . I didn't know if this was also due to the plugin cache that was incorporated in the patch. I carried over those changes by hand but no improvement to build times. I have currently reverted to the patched version of 2.2.0 for these performance reason but hope it can be sussed out prior to 2.2.1.

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 5, 2023

@ZipBrandon Here is a patch file against 2.2.0 that includes the LiveReload fix. I made a PR yesterday for my other changes, so if they get accepted before 2.2.1 you won't need this patch anymore.

@dmarkow
Copy link
Contributor Author

dmarkow commented Nov 10, 2023

I think this can be closed now that both the LiveReload fix and my caching PR are merged.

@dmarkow dmarkow closed this as completed Nov 10, 2023
Copy link
Contributor

🤖 Hello there,

We just published version 2.3.0-pre.0 which involves this issue. 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.3.0 which involves this issue. 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

No branches or pull requests

5 participants