#10498: Passing ugettext_lazy to related object's create() doesn't work
-------------------------------------+-------------------------------------
Reporter: mtredinnick | Owner: ojii
Type: Bug | Status: reopened
Component: Database layer | Version: SVN
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: dceu2011 | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by akaariai):
* cc: anssi.kaariainen@… (added)
* status: closed => reopened
* resolution: fixed =>
* severity: Normal => Release blocker
Comment:
While working on #17030 I noticed the isinstance(val, Proxy) calls in the
model.`__init__`. I did some benchmarking, and confirmed the patch applied
in this ticket causes some performance regressions. Using djangobench:
{{{
Running 'query_all' benchmark ...
Min: 0.021516 -> 0.024808: 1.1530x slower
Avg: 0.026032 -> 0.030944: 1.1887x slower
Significant (t=-5.668286)
Stddev: 0.00516 -> 0.00696: 1.3479x larger (N = 100)
Running 'query_all_multifield' benchmark ...
Min: 0.043492 -> 0.053683: 1.2343x slower
Avg: 0.049113 -> 0.061782: 1.2580x slower
Significant (t=-11.048640)
Stddev: 0.00686 -> 0.00919: 1.3382x larger (N = 100)
}}}
When repeating the query_all_multifield the result seems to stay around
20% slower (mentioning this because for some reason or other djangobench
hasn't been that reliable).
I am going to reopen and mark this ticket as a release blocker. The reason
is that I think this should be reconsidered in the light of the 20%
regression for fetching objects from the db. It seems pretty clear that
the performance regression wasn't taken in account when doing the commit,
so reconsidering this in light of the regression should be done before
1.4. I am not saying that this must be reverted, just that reconsidering
the value of the patch with the new info available should be done.
My initial feeling is that the patch should be reverted, as using
ugettext_lazy in models doesn't sound more important than the 20%
regression. There are also a couple of other ways forward:
- revert just the part of the commit which touches the args based
setattr "fast path" loop. This leaves a bit of inconsistency, but args
based `__init__` + ugettext_lazy isn't that common really.
- check isinstance(Promise) when saving the model. That way .create()
would work with ugettext lazy objects. I think there is still an unfixed
problem if you just assign a Promise object to some field, then save (no,
I haven't tested this).
Attached is the full benchmark log.
--
Ticket URL: <https://code.djangoproject.com/ticket/10498#comment:9>
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 [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
