#28828: Performance improvements for build_absolute_uri
-------------------------------+--------------------------------------
Reporter: gcbirzan | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Description changed by gcbirzan:
Old description:
> In cases where a lot of URLs are generated, the performance of it is
> suboptimal.
>
> * It calls self.get_host() several times. That function does a lot of
> work that isn't needed more than once.
> * It calls urljoin() even on the trivial case of absolute paths (which is
> the most common case). urljoin is quite expensive as well.
>
> My patch fixes these by using a cached_property for the scheme://host
> part and by just concatenating the current scheme/host with the location
> when the location is absolute, doesn't change the host, and doesn't
> contain '.' or '..'. The last can be implemented in there, but that will
> just complicate the code I think, for again, a not very common case.
>
> All the tests pass, but they did even when I wasn't checking for '.' and
> '..', so I added a test for that too.
>
> While the improvements might be minor for some use cases, an example of
> this being slow can be found in this [https://github.com/encode/django-
> rest-framework/issues/3007#issuecomment-288332131 DRF ticket].
New description:
In cases where a lot of URLs are generated, the performance of it is
suboptimal.
* It calls self.get_host() several times. That function does a lot of work
that isn't needed more than once.
* It calls urljoin() even on the trivial case of absolute paths (which is
the most common case). urljoin is quite expensive as well.
My patch fixes these by using a cached_property for the scheme://host part
and by just concatenating the current scheme/host with the location when
the location is absolute, doesn't change the host, and doesn't contain '.'
or '..'. The last can be implemented in there, but that will just
complicate the code I think, for again, a not very common case.
All the tests pass, but they did even when I wasn't checking for '.' and
'..', so I added a test for that too.
While the improvements might be minor for some use cases, an example of
this being slow can be found in this [https://github.com/encode/django-
rest-framework/issues/3007#issuecomment-288332131 DRF ticket].
I've made a [https://github.com/django/django/pull/9376 PR] with these
changes.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28828#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/066.7fb4129268de8dd510b1efc47e6b5af3%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
