Passing ugettext_lazy to related object's create() doesn't work

[Malcolm Tredinnick]

As originally reported in #10491, doing this appears to fail (unconfirmed by me at the moment):

user.message_set.create(message=ugettext_lazy(u'xxx')) 

It should work.

[liangent]

note that the problem is:

something like <django.utils.functional.__proxy__ object at 0x0110ED10> shows when i'm trying to access messages in template context messages.

so i think there's some connection with #10491 and i put it in #10491 at first.

[Jonas Obrist]

Possible solution: In django.db.models.base.Model.init, check if 'val's that are set onto the model are django.utils.functional.Promise instances, and if so, force_unicode them before setting the attribute.

[Jonas Obrist]

a fix as discussed with russ

[Claude Paroz]

Updated to latest trunk (r17503)

[Claude Paroz]

Tested that performance are not affected:

Running benchmarks: django_test_postgresql:many_to_one
Control: Django 1.4a1 (in git branch master)
Experiment: Django 1.4a1 (in git branch 10498)

Running 'django_test_postgresql' benchmark ...
Min: 0.245787 -> 0.249683: 1.0159x slower
Avg: 0.256881 -> 0.257157: 1.0011x slower
Not significant
Stddev: 0.00550 -> 0.00291: 1.8910x smaller (N = 20)

Running benchmarks: django_test_sqlite:many_to_one
Control: Django 1.4a1 (in git branch master)
Experiment: Django 1.4a1 (in git branch 10498)

Running 'django_test_sqlite' benchmark ...
Min: 0.053780 -> 0.053740: 1.0007x faster
Avg: 0.054773 -> 0.053980: 1.0147x faster
Not significant
Stddev: 0.00186 -> 0.00040: 4.6798x smaller (N = 20)

[Jannis Leidel]

In [17641]:

Fixed #10498 -- Fixed using ugettext_lazy values when creating model instances. Thanks to Claude Paroz and Jonas Obrist.

[Anssi Kääriäinen]

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.

[Anssi Kääriäinen]

djangobench log between 1.3.1 and r17672

[Anssi Kääriäinen]

The attached patch does the Promise -> unicode conversion much later in the saving cycle. It fixes numerous other ways to pass Promise objects to the DB layer, and in addition the slowdown to model init is now removed.

[Anssi Kääriäinen]

Alternate way to fix Promise object handling

[Claude Paroz]

Sorry about my benchmarks in comment:7, I was probably not targeting enough the object creation part in the executed tests. I can confirm Anssi's benchmarks, even if for query_all tests, I only get about 7% slower results.

[Anssi Kääriäinen]

It would be nice to have continuous benchmarking system...

The 5-20% slowdown is somewhat important to deal with, but now that I look at this, the _real_ reason to fix this in the subqueries module is that it is a better place to do the fix. It fixes some situations the original patch doesn't fix. I wonder if the proper place would be in database adapters actually. I don't think now is the time to do that change anyways.

My opinion is that the alternate patch should be committed both because it fixes a performance regression, and also because it fixes more cases to begin with.

[Jannis Leidel]

FTR, there is ​http://django-speedcenter.improbable.org/

[Jonas Obrist]

The new patch looks good to me. Seems reasonable and djangobench reports it fixes the performance regression.

[Jannis Leidel]

In [17698]:

Fixed #10498 (again) -- Made sure the improvements done in r17641 have a smaller impact on speed. Thanks to Anssi Kääriäinen for the patch and Jonas Obrist for reviewing.