createRequestHandler in server-runtime causing slower responses in larger apps

[dmarkow]

What version of Remix are you using?

1.8.0

Steps to Reproduce

My production app has a large number of routes (around 700). I'm dealing with some slower requests, so I ran some benchmarking to see how long remix takes to process each request, excluding time for my loaders/actions. I narrowed down the bulk of the extra processing time to the use of unstable_createStaticHandler in @remix-run/server-runtime (which delegates to @remix-run/router). Even though this function returns the same thing given the same loadContext, it's being called on every request.

Expected Behavior

In production, the results of unstable_createStaticHandler should either be calculated once at build time, or cached and only updated when the loadContext changes.

Actual Behavior

unstable_createStaticHandler is called for every request. In my 700-route app, which is adding up to 200ms to my requests.

// packages/remix-server-runtime/server.ts

export const createRequestHandler: CreateRequestHandlerFunction = (
  build,
  mode
) => {
  let routes = createRoutes(build.routes);
  let serverMode = isServerMode(mode) ? mode : ServerMode.Production;

  return async function requestHandler(request, loadContext = {}) {
    let url = new URL(request.url);
    let matches = matchServerRoutes(routes, url.pathname);

    // This is being called for every request. In a large app, this single line can easily add
    // 100-200ms of overhead.
    let staticHandler = unstable_createStaticHandler(
      createStaticHandlerDataRoutes(build.routes, loadContext)
    );

I just set up a local patch which memoizes unstable_createStaticHandler. This immediately dropped my requests from around 250ms each to around 80ms each (latency to my server is 60ms so this is around 20ms for remix to handle the request instead of almost 200ms).

[brophdawg11]

Thanks @dmarkow! Are you using the loadContext in your app? Do you want to share your patch and we will look into improving the perf there 👍

[dmarkow]

I added a cache that only gets updated in production. Digging into the internals, I'm always a little worried I may be breaking something I'm unaware of. I don't personally use loadContext but this approach should allow for creating a new handler when the context changes.

diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts
index feea61bd..6b77a8ba 100644
--- a/packages/remix-server-runtime/server.ts
+++ b/packages/remix-server-runtime/server.ts
@@ -30,6 +30,8 @@ export type CreateRequestHandlerFunction = (
   mode?: string
 ) => RequestHandler;

+let handlerCache: Record<string, StaticHandler> = {};
+
 export const createRequestHandler: CreateRequestHandlerFunction = (
   build,
   mode
@@ -41,9 +43,16 @@ export const createRequestHandler: CreateRequestHandlerFunction = (
     let url = new URL(request.url);
     let matches = matchServerRoutes(routes, url.pathname);

-    let staticHandler = unstable_createStaticHandler(
-      createStaticHandlerDataRoutes(build.routes, loadContext)
-    );
+    let key = JSON.stringify([build.routes, loadContext]);
+    let staticHandler = handlerCache[key];
+    if (!staticHandler) {
+      staticHandler = unstable_createStaticHandler(
+        createStaticHandlerDataRoutes(build.routes, loadContext)
+      );
+      if (serverMode === ServerMode.Production) {
+        handlerCache[key] = staticHandler;
+      }
+    }

     let response: Response;
     if (url.searchParams.has("_data")) {

I can formalize this into a PR if it seems like the right approach.

[dmarkow]

Actually, the let handlerCache... can probably go under the createRequestHandler function instead, and the production check can be removed. createRequestHandler only gets called repeatedly in development. And since it will only cache in production, build.routes probably doesn't need to be part of the key either.

diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts
index feea61bd..19b1bd02 100644
--- a/packages/remix-server-runtime/server.ts
+++ b/packages/remix-server-runtime/server.ts
@@ -36,14 +36,20 @@ export const createRequestHandler: CreateRequestHandlerFunction = (
 ) => {
   let routes = createRoutes(build.routes);
   let serverMode = isServerMode(mode) ? mode : ServerMode.Production;
+  let handlerCache: Record<string, StaticHandler> = {};

   return async function requestHandler(request, loadContext = {}) {
     let url = new URL(request.url);
     let matches = matchServerRoutes(routes, url.pathname);

-    let staticHandler = unstable_createStaticHandler(
-      createStaticHandlerDataRoutes(build.routes, loadContext)
-    );
+    let key = JSON.stringify(loadContext);
+    let staticHandler = handlerCache[key];
+    if (!staticHandler) {
+      staticHandler = unstable_createStaticHandler(
+        createStaticHandlerDataRoutes(build.routes, loadContext)
+      );
+      handlerCache[key] = staticHandler;
+    }

     let response: Response;
     if (url.searchParams.has("_data")) {

[dmarkow]

Instead of caching the results, we could fix createStaticHandlerDataRoutes to run quicker. Similar to my PR on #4538, the issue is repeatedly filtering the entire list of routes. Creating a map of routes by parentId and using that instead improved runtime of that function from 100ms+ to 1ms in my app:

diff --git a/packages/remix-server-runtime/routes.ts b/packages/remix-server-runtime/routes.ts
index 4dceb42d..27c87b57 100644
--- a/packages/remix-server-runtime/routes.ts
+++ b/packages/remix-server-runtime/routes.ts
@@ -57,10 +57,26 @@ export function createRoutes(
 export function createStaticHandlerDataRoutes(
   manifest: ServerRouteManifest,
   loadContext: AppLoadContext,
-  parentId?: string
+  parentId?: string,
+  routesByParentId?: Record<string, Omit<ServerRoute, "children">[]>
 ): AgnosticDataRouteObject[] {
-  return Object.values(manifest)
-    .filter((route) => route.parentId === parentId)
+  // Create a map of routes by parentId to use recursively instead of
+  // repeatedly filtering the manifest.
+  if (!routesByParentId) {
+    let routes: Record<string, Omit<ServerRoute, "children">[]> = {};
+
+    Object.values(manifest).forEach((route) => {
+      let parentId = route.parentId || "";
+      if (!routes[parentId]) {
+        routes[parentId] = [];
+      }
+      routes[parentId].push(route);
+    });
+
+    routesByParentId = routes;
+  }
+
+  return (routesByParentId[parentId || ""] || [])
     .map((route) => {
       let commonRoute = {
         // Always include root due to default boundaries
@@ -103,7 +119,8 @@ export function createStaticHandlerDataRoutes(
             children: createStaticHandlerDataRoutes(
               manifest,
               loadContext,
-              route.id
+              route.id,
+              routesByParentId
             ),
             ...commonRoute,
           };

[dmarkow]

Moving this over to a PR since I think I finally have this narrowed down to the correct set of updates.

[brophdawg11]

Thanks for looking into this @dmarkow! We discussed this a bit internally yesterday and I think we're looking at an API change over in @remix-run/router that would allow the load context to be passed into static handler loaders/actions directly, instead of being captured in the closure scope as it is today. This would lift both those problematic calls out of the requestHandler scope:

export const createRequestHandler: CreateRequestHandlerFunction = (
  build,
  mode
) => {
  let routes = createRoutes(build.routes);
  let serverMode = isServerMode(mode) ? mode : ServerMode.Production;
  let staticHandler = unstable_createStaticHandler(
    createStaticHandlerDataRoutes(build.routes)
  );

  return async function requestHandler(request, loadContext = {}) {
    let url = new URL(request.url);
    let matches = matchServerRoutes(routes, url.pathname);
    ...
  }
}

Tat should alleviate the perf concern here (as we'd be back to the way Remix did this prior to 1.8.0).

The optimization you have for route creation using a parent map is still potentially useful, but would only speed up server startup a tiny bit since it wouldn't be on the request path anymore. Do you want to trim your PR down to just that and I'll see how the team feels about that optimization?

[dmarkow]

I'll update the PR to remove the caching logic in createRequestHandler. As you said, the updates in routes.ts would really just speed up server startup a tiny bit in production, however it would provide a speed up on all requests in development.

I'm also open to shelving the whole thing for now until the "react router-ing remix" transition is done and that API change happens, then I can revisit and see if it's necessary.

[brophdawg11]

Yeah the development flow is a good point. We'll aim to get the lifting of unstable_createStaticHandler and createStaticHandlerDataRoutes in soon, and then I think it makes sense to hold on the route creation optimization until we finish getting react router 6.4 into Remix

[brophdawg11]

@dmarkow This is now released in a 1.8.2-pre.1 prerelease if you want to give that a shot and confirm it gets rid of the perf regression

[dmarkow]

@brophdawg11 This works great in production. On1.8.1 a super basic loader is taking 250-300ms in production, 1.8.2-pre.1 drops that back down to 80-100ms (again, accounting for 60ms in latency to my server).

As discussed above, there's still a slight dev delay. But it actually turns out most of that issue in dev was using Sentry.init(...), since it wraps the loaders/actions even if enabled: false is set in development. So there's still a +/- 10ms dev delay per request, but not the 100+ms I was seeing before. Thanks!

[brophdawg11]

This is released in 1.8.2. @dmarkow Let's revisit that route generation optimization once we finish the 6.4 migration - we're hoping to have that done in the next couple weeks so maybe we can loop back in January?

[dmarkow]

@brophdawg11 Sure thing! Definitely interested in helping any way I can with performance.

[github-actions]

🤖 Hello there,

We just published version v0.0.0-nightly-79859c9-20230203 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!