-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor(router-core): add LRU cache in front of parsePathname #4752
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
Conversation
View your CI Pipeline Execution ↗ for commit 1cbf087
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-with-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
does this have any implications when running on dev? e.g. do we need to clear the cache upon hmr? also we should make the size configurable |
I don't think so. We're parsing strings, using strings as keys. If the string changes, it bypasses the cache. But if it's the same string, it can return the same result, even after a HMR, without any issue. |
I wasn't sure about this. I can't see anyone playing with this setting unless they have many routes and they actually want to try increasing its size. But for people w/ a small number of routes, I can't imagine why you would disable it. Measuring memory in the browser is kinda annoying but based on vibes I'd imagine this to have a quite minimal memory footprint. |
07ba19a
to
6be6968
Compare
We noticed that
parsePathname
is a performance bottleneck during navigation events. Here's a flamegraph from an application w/ ~300 routes around a navigation event:This PR proposes a basic least-recently-used cache implementation (that data structure is about 4-5x slower than a
Map
according to benchmarks, and it uses a little more memory).We can add caching to
parsePathname
now that what it returns is readonly (#4705), which should improve the performance of this bottleneck.The cache is set to a size of 1000. When / if we end up making a pre-built matcher (WIP #4714), we maybe can reduce this size. But
parsePathname
is still called w/ built pathnames so we probably shouldn't remove the cache entirely.When benchmarking
matchPathname
with and without the cache, we notice a ~9x increase in throughput with the cache.Assuming this 9x increase translates to a proportional reduction of the self-time seen in the flamegraph, it would go from 55ms to 6ms.