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

Fixed #35518 -- Used simpler string operations for converter-less routes #18270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RealOrangeOne
Copy link
Contributor

@RealOrangeOne RealOrangeOne commented Jun 12, 2024

Trac ticket number

ticket-35518

Branch description

The RoutePattern assumes all routes provided include some form of converter, and so needs to change it into a regex for matching. However, if no converters are included in the string, the additional overhead of using a regex vs simpler string operations is unnecessary.

Replacing this with a simpler string comparison results in between a 50 and 75% reduction in match time, which stacks up quickly as an application generally has numerous URLs, because RoutePattern.match is called once per defined route per request.

Before

In [2]: endpoint_pattern = RoutePattern("foo/", "name", is_endpoint=True)

In [3]: pattern = RoutePattern("foo/", "name", is_endpoint=False)

In [4]: %timeit pattern.match("foo/")
441 ns ± 2.68 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

In [5]: %timeit endpoint_pattern.match("foo/")
435 ns ± 0.974 ns per loop (mean ± std. dev. of 7 runs, 1,000,000 loops each)

After

In [4]: %timeit pattern.match("foo/")
187 ns ± 1.84 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

In [5]: %timeit endpoint_pattern.match("foo/")
103 ns ± 0.192 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

(Benchmarked on Python 3.12, running main)

Theoretically, these improvements get better based on the length of the route pattern (although at this scale, not notably).

This optimisation could be done by adding a different kind of pattern (eg LiteralPattern), but the added complexity to a project probably isn't necessary, not to mention the migration effort to take advantage of this.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have checked the "Has patch" ticket flag in the Trac system.
  • I have added or updated relevant tests.
  • I have added or updated relevant docs, including release notes if applicable.
  • I have attached screenshots in both light and dark modes for any UI changes.

@RealOrangeOne RealOrangeOne marked this pull request as draft June 12, 2024 19:23
@RealOrangeOne RealOrangeOne force-pushed the 35518-route-pattern-string branch 2 times, most recently from 9416613 to b0bbb03 Compare June 12, 2024 19:39
django/urls/resolvers.py Outdated Show resolved Hide resolved
django/urls/resolvers.py Outdated Show resolved Hide resolved
@smithdc1 smithdc1 added the benchmark Apply to have ASV benchmarks run on a PR label Jun 13, 2024
@RealOrangeOne RealOrangeOne marked this pull request as ready for review June 19, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Apply to have ASV benchmarks run on a PR
Projects
None yet
3 participants