django/db/models/base.py uses inefficient subclass check

[Ludvig Ericson <ludvig.ericson@…>]

django/db/models/base.py uses

    filter(lambda b: issubclass(b, Model), bases)

to check if a model inherits from Model, which is inefficient (creates a list when it doesn't need one)
The supplied patch provides a more efficient version.

[Adrian Holovaty]

Hi Ludvig,

Django is committed to using Python 2.3, and this patch uses the any keyword, which was introduced in Python 2.5. Could you submit a patch that doesn't use any?

[Ludvig Ericson <ludvig.ericson@…>]

Hmm, you're right.

Worse yet, generator expressions do not exist in Python 2.3.

Supplied patch uses an old fashion for construct.

[Ludvig Ericson <ludvig.ericson@…>]

Warning: The patch has reversed logics, don't use it. I'm fixing this right now.

[Ludvig Ericson <ludvig.ericson@…>]

Hopefully last patch, it works for me quite well. It is a bit ugly, though.

Anyway, base should always be set since the new is only called when there are base-classes, so there should be at least one item in that tuple. Maybe even

    base = None

before the for loop.

[Chris Beaven]

Logic still looks wrong to me. base will never be not None

[anonymous]

It will be not None when the loop exits, since python - intentionally - leaks the variable into the containing scope.

[Michael Radziej <mir@…>]

Replying to anonymous:

It will be not None when the loop exits, since python - intentionally - leaks the variable into the containing scope.

You're right. But it would be much clearer without break, wouldn't it?

for base in bases: 
  if issubclass(base, Model):
    return ...

I really don't like this leaking out, but perhaps it's a matter of taste ;-)

[ludvig.ericson@…]

Hrm, you're missing the logic.
If the class is _not_ a subclass of Model, it should be returned early - without adding various attributes to the class.

[Michael Radziej <mir@…>]

Sorry ;-)

Then:

for base in bases:
  if issubclass(base, Model):
    break
else:
  return

[Ludvig Ericson <ludvig.ericson@…>]

Wow. I didn't know that worked in python. I had been pondering that myself for quite some time already, thanks for the enlightenment!

I'll attach a new patch in a few minutes.

[Malcolm Tredinnick]

There's been a lot of effort put into this ticket, but I can't see the benefit. The speed up exists, but it's very slight (about 15% on the timings I've done -- and the absolute elapsed time is less than a microsecond on modern day machines). This particular line of code is executed precisely once for each model when the file that defines it is imported (the metaclass __new__ method is not called when instances are created, only when the class object is created). Replacing one line of code with three when the existing single line seems pretty clear isn't worth it.

Sorry, guys, but this isn't really worth changing.