Opened 8 years ago

Closed 8 years ago

#4050 closed (wontfix)

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

Reported by: Ludvig Ericson <ludvig.ericson@…> Owned by: adrian
Component: Database layer (models, ORM) Version: master
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: UI/UX:

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@…> 8 years ago.
base.py.2.diff (793 bytes) - added by Ludvig Ericson <ludvig.ericson@…> 8 years ago.
base.py.3.diff (780 bytes) - added by Ludvig Ericson <ludvig.ericson@…> 8 years ago.
base.py.4.diff (714 bytes) - added by Ludvig Ericson <ludvig.ericson@…> 8 years ago.

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by Ludvig Ericson <ludvig.ericson@…>

comment:1 Changed 8 years ago by adrian

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 8 years ago by 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.

Changed 8 years ago by Ludvig Ericson <ludvig.ericson@…>

comment:3 Changed 8 years ago by Ludvig Ericson <ludvig.ericson@…>

  • Patch needs improvement set

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

Changed 8 years ago by Ludvig Ericson <ludvig.ericson@…>

comment:4 Changed 8 years ago by 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.

comment:5 Changed 8 years ago by SmileyChris

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

comment:6 follow-up: Changed 8 years ago by anonymous

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

comment:7 in reply to: ↑ 6 Changed 8 years ago by 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 ;-)

comment:8 Changed 8 years ago by 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.

comment:9 Changed 8 years ago by Michael Radziej <mir@…>

Sorry ;-)

Then:

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

comment:10 Changed 8 years ago by 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.

Changed 8 years ago by Ludvig Ericson <ludvig.ericson@…>

comment:11 Changed 8 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed

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