Opened 14 years ago

Closed 8 years ago

#14297 closed Cleanup/optimization (duplicate)

Accessing settings.FOO in hot spots cause performance problems

Reported by: Anssi Kääriäinen Owned by:
Component: Core (Other) Version: dev
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 14 years ago.
ticket_14297.diff (5.9 KB ) - added by Luke Plant 14 years ago.

Download all attachments as: .zip

Change History (13)

by Anssi Kääriäinen, 14 years ago

Attachment: settings_optimization.diff added

comment:1 by Jannis Leidel, 14 years ago

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 by anonymous, 14 years ago

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 by Luke Plant, 14 years ago

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.

by Luke Plant, 14 years ago

Attachment: ticket_14297.diff added

comment:4 by Alex Gaynor, 14 years ago

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.

in reply to:  4 comment:5 by Luke Plant, 14 years ago

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 by Alex Gaynor, 14 years ago

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 by Julien Phalip, 13 years ago

Severity: Normal
Type: Cleanup/optimization

comment:8 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 by Luke Plant, 11 years ago

Owner: Luke Plant removed
Status: assignednew

comment:11 by Adam Johnson, 8 years ago

Resolution: duplicate
Status: newclosed

Duplicate of #27625

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