launchpad-dev team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

On Wed, May 4, 2011 at 10:32 PM, Gavin Panella
<gavin.panella@xxxxxxxxxxxxx> wrote:
> One common performance killing pattern is dereferencing non-cached
> References on a sequence of model objects. A stupid example might be:
>
>    owners = [bug.owner for bug in lots_of_bugs]
>
> Instead we're learning to write this like:
>
>    owners = bulk.load_related(Person, lots_of_bugs, "ownerID")

I suspect this should be a Storm feature. Something like
Bug.owner.load(lots_of_bugs)


> One approach to improving this situation could be to forbid (with
> code) the use of Reference properties that are not already cached (in
> the store cache; this is nothing to do with @cachedproperty).

Rather than forbid, we could warnings.warn(). This will break tests,
but continue to work live for any untested code paths that sneak
through.

> Doing this everywhere is not going to fly, at least right now -
> there's probably way too much code that would break if we did this -
> and sometimes we would want to permit their use - when we're just
> looking up a single object for example.

Adding a ratchet might help for the broken tests. ie. only raise that
warning the 10th time we dereference, resetting the counter in
DatabaseLayer.testTearDown().

> Before taking this any further I am interested in your views. Can you
> think of places where you could, or could have, used this? Is the
> granularity right, or should we be able to restrict just individual
> properties, or all properties on a model class? Is the idea okay but
> the implementation going about it all wrong?

Enforcing this will catch issues, but I can't see how to enforce it
without also requiring the bulk load syntax be used for accessing a
single item. I suspect this would be an awful lot of fallout in the
current code base, and a lot of typing to fix as I don't think we can
use a script to migrate the code.


-- 
Stuart Bishop <stuart@xxxxxxxxxxxxxxxx>
http://www.stuartbishop.net/