Opened 17 years ago

Closed 17 years ago

#4050 closed (wontfix)

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

Reported by: Ludvig Ericson <ludvig.ericson@…> Owned by: Adrian Holovaty
Component: Database layer (models, ORM) Version: dev
Severity: Keywords: issubclass models base.py
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (4)

base.py.diff (658 bytes ) - added by Ludvig Ericson <ludvig.ericson@…> 17 years ago.
base.py.2.diff (793 bytes ) - added by Ludvig Ericson <ludvig.ericson@…> 17 years ago.
base.py.3.diff (780 bytes ) - added by Ludvig Ericson <ludvig.ericson@…> 17 years ago.
base.py.4.diff (714 bytes ) - added by Ludvig Ericson <ludvig.ericson@…> 17 years ago.

Download all attachments as: .zip

Change History (15)

by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

Attachment: base.py.diff added

comment:1 by Adrian Holovaty, 17 years ago

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?

comment:2 by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

Hmm, you're right.

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

Supplied patch uses an old fashion for construct.

by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

Attachment: base.py.2.diff added

comment:3 by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

Patch needs improvement: set

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

by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

Attachment: base.py.3.diff added

comment:4 by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

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.

comment:5 by Chris Beaven, 17 years ago

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

comment:6 by anonymous, 17 years ago

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

in reply to:  6 comment:7 by Michael Radziej <mir@…>, 17 years ago

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 ;-)

comment:8 by ludvig.ericson@…, 17 years ago

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.

comment:9 by Michael Radziej <mir@…>, 17 years ago

Sorry ;-)

Then:

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

comment:10 by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

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.

by Ludvig Ericson <ludvig.ericson@…>, 17 years ago

Attachment: base.py.4.diff added

comment:11 by Malcolm Tredinnick, 17 years ago

Resolution: wontfix
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top