severe performance issue regarding mutable_fields

[simion]

The mutable_fields functionality has some serious performance issues. Precisely, overriding the def __getattribute__(self, name) is the problem.

I don't use the "SaveTheChange" mixin, (i only use "TrackChanges" (i check if some fields have changed, in signals), and as far as i see, mutable_fields is only used to correctly build the "update_fields" list on SaveTheChange mixin, save method.

I recommend moving all the functionality regarding mutable_fields to SaveTheChange untill you figure out what's wrong with__getattribute__ and why it is so slow.

The project on wich I use your mixins is quite large, and i just saved more than 2 seconds of page loading time by commenting out all mutable_fields related stuff.

[karanlyons]

Yeah, I bet you’re running into the pathological case of recursive calls through is_mutable. Sorry.

I think I’m going to attempt to use __getattribute__ only when it would be necessary (it exists to handle cases where dev may make changes to a mutable type/object, which doesn’t result in a __setattr__ call. We therefore have no way of knowing if there’s been an alteration, so want to send that data back up to the db regardless), and additionally add a way to explicitly state via a Model's meta which fields should get this extra work applied to them.

[simion]

I think specifying the fields that are mutable (or those that are imutable) will do the trick.

FYI, I've done some tests with a basic class, calling an attribute 10 milion times (the attribute was an integer). It took 35ms. I implemented getattribute special method, and returned super, and it took 550ms. This does not apply to other methods. A basic method override did not consume almost no extra time.

Just wanted to let you know, maybe we can find another way of detecting changes on mutable fields.

Simion Agavriloaei

[karanlyons]

There’s also a nasty performance penalty in that the whole body of __getattribute__ including the is_mutable calls happens on every access as opposed to just the first call. That one fix would likely help substantially, although the addition of optional and explicit declarations by the developer would surely help as well.

I had this nasty idea of recursively wrapping immutable iterables to allow mutations within them to “bubble up” as it were, but that’d require wrapping the iterables and anything they contained at object instantiation, which is far too much overhead for attributes that may never even be modified in the first place.

[karanlyons]

Hi! Long time no chat. How’ve you been?

The best way to do this would be with a metaclass that checks at class creation if that model has any mutable fields (either defined explicitly as a meta option, or through “magic”). Metaclassing makes me want to cry, and so my first little stab at this is instead this:

1. Rename __getattribute__ in BaseChangeTracker to ___getattribute__ so that it’s not used by default.
2. Add to BaseChangeTracker’s __init__:

for field in self._meta.concrete_fields:
    if is_mutable(field.get_default()):
        self.__getattribute__ = self.___getattribute__
        break

So you’re still paying a check at instance creation instead of class creation, but not over the lifetime of the instance’s use. Which is kinda better, in the “perfect is the enemy of good” sort of way. If we’d want to continue doing this at instance creation it’d likely be better to actually check the instance’s values themselves as well as the defaults, but what I’d really prefer is to do it at the class level and rely on the developers to themselves decide which fields they know to be mutable, and additionally whether they really want the overhead required to send them only when they change as opposed to just every time.

[karanlyons]

Okay, I’ve switched the whole thing over to a decorator model which is much more efficient (and maintainable) whilst still giving us the tracking of mutable fields. So I’m marking this fixed, unless you pull down master and find out I’m completely wrong 😄