T
TanStack•2mo ago
correct-apricot

Performance issue w/ `parsePathname` in `path.ts`

I tried to upgrade from 1.120.18 to 1.126.2. We had been stuck for a while on 1.120 because of the false positive "could not find match from" issue that got solved yesterday (https://github.com/TanStack/router/pull/4610). But in the CI for this upgrade, all our E2E were timing out. I tried it, it does feel very sluggish. It turns out that there seems to be a big performance regression in path.ts where the parsePathname function takes a lot of time (and is called a lot). Here's a performance trace from the chrome devtools on a staging build (not local dev, just to be sure I'm measuring something prod-like).
No description
No description
4 Replies
correct-apricot
correct-apricotOP•2mo ago
The anonymous function at the top of the call-list when sorting by self-time is the split.map callback inside parsePathname I haven't looked at the source code yet, but I could imagine a couple avenues to improve this situation: - some amount of caching. We're probably calling this with the same few pathname most of the time, and this is a pure function, so caching could be extremely effective - reordering the tests inside the map, to favor the "most common" ones. Most segments probably aren't dynamic, if they are they probably don't have a prefix/suffix Ok I found where the perf issue is coming from in our codebase. It might point towards an issue in tanstack/router, but it might not. We have a hook inspired by useMatchRoute that we call useActiveRoute and looks like this: 
  const router = useRouter()

  return useRouterState({
    select: () => {
      const { pending, caseSensitive, fuzzy, includeSearch = Boolean(opts.search), ...rest } = opts
      const match =
        router.matchRoute(rest as any, {
          caseSensitive,
          fuzzy,
          includeSearch,
          pending,
        }) || undefined
      return Boolean(match)
    },
  })
  const router = useRouter()

  return useRouterState({
    select: () => {
      const { pending, caseSensitive, fuzzy, includeSearch = Boolean(opts.search), ...rest } = opts
      const match =
        router.matchRoute(rest as any, {
          caseSensitive,
          fuzzy,
          includeSearch,
          pending,
        }) || undefined
      return Boolean(match)
    },
  })
It turns out, somewhere between 1.120 and 1.126, the select of useRouterState started being called much more during a navigation. This part is a little fuzzy, but that's exactly the cause of the perf issue. It runs more, and matchRoute is expensive because at some point it does a .find() on the array of all routes. The perf issue can be solved by not using the select: 
  const router = useRouter()

  useRouterState({
    select: (s) => [s.location.href, s.resolvedLocation?.href, s.status],
    structuralSharing: true as any,
  })

  const { pending, caseSensitive, fuzzy, includeSearch = Boolean(opts.search), ...rest } = opts
  const match =
    router.matchRoute(rest as any, {
      caseSensitive,
      fuzzy,
      includeSearch,
      pending,
    }) || undefined
  return Boolean(match)
  const router = useRouter()

  useRouterState({
    select: (s) => [s.location.href, s.resolvedLocation?.href, s.status],
    structuralSharing: true as any,
  })

  const { pending, caseSensitive, fuzzy, includeSearch = Boolean(opts.search), ...rest } = opts
  const match =
    router.matchRoute(rest as any, {
      caseSensitive,
      fuzzy,
      includeSearch,
      pending,
    }) || undefined
  return Boolean(match)
now this finding 100% solves my perf issue, so I'm all good. But the fact that the select runs too often during a navigation probably means something like: we update a synchronous external store several times during a single "action". This might not be desirable?
correct-apricot
correct-apricot•2mo ago
your inital hook did not use structuralSharing why is that? oh you did not need it because of a boolean we are indeed often updating the store the select function is not meant for "heavy" computations but why do you manually need to match routes?
correct-apricot
correct-apricotOP•2mo ago
It's for our menu. Our URLs aren't neatly nested: /a/b/c and /foo/bar might both count towards /hello being "active"
correct-apricot
correct-apricot•2mo ago
but could you not have a mapping of routes that you lookup? would need a complete example to understand your setup

Did you find this page helpful?