Opened 6 years ago

Last modified 3 years ago

#14297 new Cleanup/optimization

Accessing settings.FOO in hot spots cause performance problems

Reported by: Anssi Kääriäinen Owned by:
Component: Core (Other) Version: master
Severity: Normal Keywords: settings, performance
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Trying to access settings.FOO (no matter if it exists or not) will cause performance problems if used in hot spots of code. The reason is that settings is actually LazyObject, and each access goes through getattr. The problem is well illustrated in ticket #14290, where performance is increased considerably if a setting is cached in a global attribute.

I am trying to fix this by refactoring LazySettings in the following way: Make the new LazySettings be a regular Python object, but with a special getattr method: When first accessed, it will set up the settings in the objects own dict. After this, each access to settings.FOO, where FOO exists, will not use getattr. When trying to access settings.BAR, where BAR does not exist, the access will still need to go through getattr. This should not be a performance problem, though.

The performance increase on my laptop (using the test from #14290, with USE_L10N = False) is from 1.1 to 0.8 seconds, or 0.3 seconds. This is the cost of accessing settings.USE_L10N 60000 times and settings.USE_I18N 10000 times, plus fifteen other accesses. (The test is the http://localhost:8000/test/10000?timing one).

Attached patch passes all tests (I know how to run).

Attachments (2)

settings_optimization.diff (6.3 KB) - added by Anssi Kääriäinen 6 years ago.
ticket_14297.diff (5.9 KB) - added by Luke Plant 6 years ago.

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by Anssi Kääriäinen

Attachment: settings_optimization.diff added

comment:1 Changed 6 years ago by Jannis Leidel

Needs documentation: unset
Needs tests: unset
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Accepting this to acknowledge that this could be optimized. The attached patch seems rather obscure though.

comment:2 Changed 6 years ago by anonymous

Another idea how to handle this. Currently, settings is a LazySettings object, which in turn is a LazyObject. LazyObject works like this: when an attribute lookup comes in to __getattr__, then if the LazyObject has a initialized _wrapped object, the __getattr__ will be passed to the _wrapped object. Now, this could be changed (for settings only) so that when something comes to __getattr__, the value is looked in the _wrapped object, and then set into the LazyObject itself. Thus, any subsequent lookup will not go through __getattr__, but instead be a direct attribute lookup. This will of course have the problem that changing settings when testing will not work. But as far as I understand there should be a better solution (to be invented) to the changing settings for testing problem. That is, settings can be changed only for testing, and otherwise they should be static throughout the whole life of the Django instance.

Just to make sure the idea is understood:

In django.conf.__init__.py, instead of having class LazySetting(LazyObject), have class LazySetting(LazySettingWrapper). Leave everything else as is. LazySettingsWrapper is defined as follows:

class LazySettingsWrapper(LazyObject):
    def __getattr__(self, name):
        if self._wrapped is None:
            self._setup()
        val = getattr(self._wrapped, name)
        # Set the attribute so that subsequent calls will
        # get the value directly from this object, instead
        # of going through __getattr__.
        setattr(self, name, val)
        return val

But as said before, this still has the ugly "changing settings for testing" problem. Maybe the best solution to this would be to have a flush_settings method in the settings object. This method could also handle clearing possibly settings-dependent cached values.

comment:3 Changed 6 years ago by Luke Plant

Owner: changed from Anssi Kääriäinen to Luke Plant
Status: newassigned

I don't know what you mean by the "changing settings for testing" problem - it's perfectly fixable by implementing setattr properly. Or, even better, a complete re-implementation of LazySettings that dodges most of the problems: when __getattr__ is first accessed, we run _setup() which loads all the values from the actual settings object in the LazySettings dict.

I've implemented in the attached patch. I had to fix a few tests that were naughtily accessing the ._wrapped attribute, which no longer exists, but otherwise no changes will be necessary - this is backwards compatible.

For performance, this approach might give a very slight performance hit on startup, since the whole dictionary of settings has to be copied as soon as the settings are configured. But afterwards it should eliminate a fair bit of overhead when settings are used in a loop. I've created a djangobench benchmark and can verify a 30% improvement with this patch. Other benchmarks give a small improvement (about 2%) or an insignificant change.

Changed 6 years ago by Luke Plant

Attachment: ticket_14297.diff added

comment:4 Changed 6 years ago by Alex Gaynor

This latest patch works, except one problem: if the underlying module is changed it won't be reflected. Not sure if that's a semantic that was guaranteed before or not.

comment:5 in reply to:  4 Changed 6 years ago by Luke Plant

Replying to Alex:

This latest patch works, except one problem: if the underlying module is changed it won't be reflected. Not sure if that's a semantic that was guaranteed before or not.

I'm pretty sure we didn't guarantee that - the docs don't imply it, and state that you shouldn't change the settings at run time at all.

comment:6 Changed 6 years ago by Alex Gaynor

Another small semantic change: if you changed a settings on the settings object, it would be reflected on the underlying module, same with deletion.

comment:7 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Cleanup/optimization

comment:8 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 Changed 3 years ago by Luke Plant

Owner: Luke Plant deleted
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top