On Mon, Sep 26, 2011 at 9:50 PM, Luke Plant <l.plant...@cantab.net> wrote:
> Hi all, > > I finally got fed up with the common performance problem of ending up > doing O(n) DB queries when you need the (many) related objects of a list > of objects, and did something about it: > > https://code.djangoproject.com/ticket/16937 > > I'm pretty sure the concept is something desirable, the problem is the > implementation. > > It is much uglier than I'd like, but I think the current way that > QuerySets/Managers/related managers interact make this inevitable. > > The current behaviour also makes any kind of 3rd party monkey patching > solution extremely hard. For example, the related object descriptors > make it pretty hard to decorate instances of the queryset with some > proxy objects, because they intercept any assignment of attributes of > the same name. (i.e. if you have foo.bar_set, you can't do foo.bar_set > == some_precalculated_objects as a performance hack). And the way the > descriptors work mean that you get new manager objects (even new manager > *classes*) every time you access the property, and this really works > against any kind of monkey patching. > > If you want to implement this outside core, and you don't want to change > templates and other code, I think you'd have to create a proxy for the > entire model instance, and I imagine it would be a total pain. > > So, given how common this performance problem is, and how hard it is to > implement outside core, I think we should consider even an ugly > implementation. I think it is also inevitable that it is going to > involve QuerySet/related managers hacking each other's internals, or > duplicating to some extent. In my implementation, I put most of the > ugliness into QuerySet, and a few hooks into related managers. It might > be possible to clean it up a bit. > > There is also a fairly nasty use of .extra() which is needed so that > after we retrieve M2M related objects in bulk we know which row on the > primary table they refer to. > > I've got tests and docs in the patch on the ticket. I would say it is > more than proof-of-concept at this stage, but probably less than > finished patch - I may have forgotten some cases that we need to cover. > > Feedback welcome! > > Regards, > > Luke > > -- > "I spilled spot remover on my dog. Now he's gone." (Steven Wright) > > Luke Plant || http://lukeplant.me.uk/ > > -- > You received this message because you are subscribed to the Google Groups > "Django developers" group. > To post to this group, send email to django-developers@googlegroups.com. > To unsubscribe from this group, send email to > django-developers+unsubscr...@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/django-developers?hl=en. > > I'm not a fan of this, for a few reasons, firstly: because it feels wrong for a QuerySet to execute multiple queries. This isn't a deal breaker, just something that struck my conceptually initially. Second I disagree that it's difficult to do outside of core, it's not as easy as it ought to be, but it's very possible (citation: I've done it :)). Finally (for now ;)) it doesn't feel right for something inside core to have caveats like "only works if you use .all()", there's a very good technical reason for this, but something about it is irking me the wrong way. Needs more thought by me'ly yours, Alex -- "I disapprove of what you say, but I will defend to the death your right to say it." -- Evelyn Beatrice Hall (summarizing Voltaire) "The people's good is the highest law." -- Cicero -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
