#16759: Expensive sql.Query cloning
-------------------------------------+-------------------------------------
Reporter: Suor | Owner: Suor
Type: Bug | Status: new
Milestone: | Component: Database layer
Version: 1.3 | (models, ORM)
Resolution: | Severity: Normal
Triage Stage: Accepted | Keywords: orm performance
Needs documentation: 0 | cloning
Patch needs improvement: 0 | Has patch: 1
UI/UX: 0 | Needs tests: 0
| Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by akaariai):
* needs_better_patch: 1 => 0
Comment:
Sounds like a much cleaner approach.
There is one problem however. django.db.model.fields.Field does already
define a `__deepcopy__` method. It is used in two places in
django/db/models/base.py:`__new__`() when inheriting fields:
{{{
# line 168
for field in parent_fields:
new_class.add_to_class(field.name, copy.deepcopy(field))
# The other one is similar, at line 189 when inheriting virtual fields.
}}}
That deepcopy does a half-shallow copy of the field. There seems to be no
other intentional uses of Field.`__deepcopy__`. It is possible to redefine
`Field`.`__deepcopy__` to return self, and rename the current deepcopy to
something else (clone?) and use that in the above snippet. A patch
implementing this approach is attached. There is some weirdness, because
`GenericForeignKey` isn't a subclass of Field, and `RelatedField` isn't a
subclass either, except it seems to somehow be a subclass (if I define a
clone which would return a deepcopy, the Field.`__deepcopy__` is called).
See lines 87-88 of django/db/models/fields/related.py. A better comment
there would be useful :)
There are also some other places that use shallow-copying deepcopy. For
example django.forms.Field, django.forms.Widget and
django.db.models.sql.Query. I guess these should be fixed too, but that is
another ticket's problem.
A completely different approach is to define .clone() for `WhereNode`. The
new method can do the minimal amount of copying needed.
How about the testing aspect? Does this need tests or is it enough to not
break the current ones? If this does need tests, then what to test? The
attached patch does not have tests. It passes all current tests. I did
some quick benchmarking, and this seems to be 2-3x faster for the simple
case of qs.filter(pk=1) 1000 times in a loop.
I hope I am helping more than distracting :)
--
Ticket URL: <https://code.djangoproject.com/ticket/16759#comment:3>
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 post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
