Skip to content

[26.0] Cache url path lookup#22198

Open
mvdbeek wants to merge 2 commits intogalaxyproject:release_26.0from
mvdbeek:cached-route-lookup
Open

[26.0] Cache url path lookup#22198
mvdbeek wants to merge 2 commits intogalaxyproject:release_26.0from
mvdbeek:cached-route-lookup

Conversation

@mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Mar 20, 2026

I've seen py-spy dumps where usegalaxy.org was busy looking up routes. This seems like an easy win. Claude claims 87X speedup in a synthetic benchmark: https://gist.github.com/mvdbeek/a257c8885edba9e02482b598076eaa0e

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

I've seen py-spy dumps where usegalaxy.org was busy looking up routes.
This is an easy win. Claude claims 87X speedup in a synthetic benchmark:
https://gist.github.com/mvdbeek/a257c8885edba9e02482b598076eaa0e
Copy link
Member

@jmchilton jmchilton left a comment

Choose a reason for hiding this comment

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

Looks cool!

Comment on lines +244 to +245
url = route.url_path_for(name, **path_params)
self._route_cache[name] = route
Copy link
Member

Choose a reason for hiding this comment

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

After reading https://github.com/Kludex/starlette/blob/main/starlette/routing.py , I think that this is not 100% safe, as there could be multiple routes with the same name and only one match with the specified path_params , so reusing the cached value for different path_params may be incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great catch, what about 70317f2 ?

Multiple routes can share a name but expect different parameter sets.
Using just the name as cache key could return the wrong route when
path_params differ. Key on (name, frozenset(path_params)) instead.
@mvdbeek mvdbeek force-pushed the cached-route-lookup branch from 70317f2 to 261634b Compare March 20, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

3 participants