Opened 5 years ago

Last modified 2 years ago

#14297 new Cleanup/optimization

Accessing settings.FOO in hot spots cause performance problems

Reported by: akaariai 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 akaariai 5 years ago.
ticket_14297.diff (5.9 KB) - added by lukeplant 5 years ago.

Download all attachments as: .zip

Change History (12)

Changed 5 years ago by akaariai

comment:1 Changed 5 years ago by jezdez

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

comment:2 Changed 5 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 5 years ago by lukeplant

  • Owner changed from akaariai to lukeplant
  • Status changed from new to assigned

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 5 years ago by lukeplant

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

comment:5 in reply to: ↑ 4 Changed 5 years ago by lukeplant

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 5 years ago by Alex

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 4 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:8 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:10 Changed 2 years ago by lukeplant

  • Owner lukeplant deleted
  • Status changed from assigned to new
Note: See TracTickets for help on using tickets.
Back to Top