[Django] #20757: A more Object-Oriented URLResolver
Django
-------------------------------+--------------------
Reporter: fcurella | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Currently, the `RegexURLResolver` is populated using tuples.
Because of that, code that tries to inspect the resolver will end up
containg awkward indexing like this (semplified code for clarity's sake):
{{{
def get_urlformat(urlname):
"""
Given a URL name, returns the URL as a string suitable for
string.format.
Example::
urlpatterns = patterns('',
url(r'^extra/(?P<extra>\w+)/$', empty_view, name="named-url2"),
)
>>> get_urlformat('named-url2')
'extra/%(extra)s/'
"""
resolver = get_resolver()
return resolver.reverse_dict[urlname][0][0][0]
}}}
My proposal is to replace tuples with a tuple-like object whose elements
can be accessed by using attribute names. That way, the above method could
become:
{{{
def get_urlformat(urlname):
"""
Given a URL name, returns the URL as a string suitable for
string.format.
Example::
urlpatterns = patterns('',
url(r'^extra/(?P<extra>\w+)/$', empty_view, name="named-url2"),
)
>>> get_urlformat('named-url2')
'extra/%(extra)s/'
"""
resolver = get_resolver()
urlbit = resolver.reverse_dict[urlname].urlbits[0]
return urlbit.format
}}}
I realize this is mostly aesthetic, and there could be performance
implications since the URLResolver is probably the most hit codepath, so I
appreciate every kind of opinion.
The attached patch is still a draft, it definitely needs more extensive
test coverage, but it's a start to get the idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/20757>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: fcurella | Owner: nobody
Type: New feature | 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
Changes (by fcurella):
* cc: flavio.curella@… (added)
* needs_better_patch: => 0
* component: Uncategorized => Core (URLs)
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => New feature
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:1>
Django
Reporter: fcurella | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by fcurella):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:2>
Django
Reporter: fcurella | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by fcurella):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:3>
Django
Reporter: fcurella | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* stage: Unreviewed => Accepted
Comment:
Hi,
Python already has a `namedtuple` class in its `collections` module which
is what you should be using for this case [1].
In theory, a namedtuple is a drop-in replacement for a regular tuple so
backwards-compatibility should be assured.
However, I'm worried about the performance cost of such a replacement: as
you noted, `RegexURLResolver` is a pretty critical component of django.
From a quick search, there's nothing in our documentation that explains
how to extend the URL resolver (or why you'd want to).
I've also never done it myself either so I don't see exactly what we'd
gain with this change.
So, to sum up, I'm -0 on the idea of using namedtuples, provided there's
no major performance hit.
On the other hand, I'd definitely be +1 on documenting the process of
extending the URL resolver and its machinery.
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:4>
Django
Reporter: fcurella | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by fcurella):
I've updated the patch to use namedtuples and rebased against
7523e784633b7757fbc82df58f80b197eeed988a.
Re performance: according to the Python docs 'Named tuple instances do not
have per-instance dictionaries, so they are lightweight and require no
more memory than regular tuples.'.
I'd still like to run some profiling to have a better idea of the impact
of this change. Memory consumption might be the same, but instantiation
could have an overhead.
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:5>
Django
Reporter: fcurella | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by UloPe):
I've done a quick benchmark. The code and raw results can be found here:
https://gist.github.com/ulope/a63aff42f51b08181f72#file-results-txt
Here is a table of the results (n=1000000):
||= Python =||= operation =||= datastructure =||= time =||
|| CPy 2.6 || instantiate || tuple || 0.243867874146 ||
|| || instantiate || namedtuple || 0.791953086853 ||
|| || access || tuple || 0.342396020889 ||
|| || access || namedtuple || 1.09635186195 ||
|| || || || ||
|| CPy 2.7 || instantiate || tuple || 0.300357103348 ||
|| || instantiate || namedtuple || 0.817892074585 ||
|| || access || tuple || 0.397746801376 ||
|| || access || namedtuple || 1.12735199928 ||
|| || || || ||
|| CPy 3.3 || instantiate || tuple || 0.19853064499329776 ||
|| || instantiate || namedtuple || 0.8839332649949938 ||
|| || access || tuple || 0.2663196460052859 ||
|| || access || namedtuple || 1.1236915799963754 ||
|| || || || ||
|| CPy 3.4 || instantiate || tuple || 0.2100249359937152 ||
|| || instantiate || namedtuple || 0.707535210007336 ||
|| || access || tuple || 0.2940493019996211 ||
|| || access || namedtuple || 0.9200455860118382 ||
|| || || || ||
|| PyPy 2.3 || instantiate || tuple || 0.00525689125061 ||
|| || instantiate || namedtuple || 0.00653004646301 ||
|| || access || tuple || 0.00449419021606 ||
|| || access || namedtuple || 0.00840997695923 ||
From this it seems that namedtuple indeed is noticeably more expensive
than a plain tuple (except, unsurprisingly, on PyPy). However as the
absolute times for namedtuple are still quite short even at one million
iterations I think the impact on Django's URL resolvers should be
negligible.
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:6>
Django
Reporter: fcurella | Owner: knbk
Type: New feature | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by knbk):
* owner: nobody => knbk
* status: new => assigned
Comment:
I'll address this as part of https://groups.google.com/forum/#!topic
/django-developers/WQoJN_MGTOg.
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:7>
Django
Reporter: fcurella | Owner: knbk
Type: New feature | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by knbk):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:8>
Django
Reporter: Flavio Curella | Owner: (none)
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Mariusz Felisiak):
* owner: Marten Kenbeek => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:9>
Django
Reporter: Flavio Curella | Owner: (none)
Type: New feature | Status: new
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20757#comment:10>