Last session refactoring vastly down Django performance

[Manuel Saelices]

Changes made in [6333] are wonderful because allow Django to store session information not only in database, rather in file system or memcached.

But the problem is Django is now too much slower (I'll have to investigate why). These are my experiments in my web site, that is more than a simple poll application:

  ~$ cd django_src
  ~$ svn up -r6332
  [...]
  ~$ ab2 -n 200 http://localhost:8000/
  [...]
  Document Path:          /
  Document Length:        8598 bytes

  [...]
  Total transferred:      966800 bytes
  HTML transferred:       928600 bytes
  Requests per second:    33.75 [#/sec] (mean)
  Time per request:       29.631 [ms] (mean)
  Time per request:       29.631 [ms] (mean, across all concurrent requests)
  Transfer rate:          159.29 [Kbytes/sec] received

  [...]

  ~$ svn up -r6333
  [...]
  ~$ ab2 -n 200 http://localhost:8000/
  [...]
  Document Path:          /
  Document Length:        8598 bytes

  [...]
  Total transferred:      966800 bytes
  HTML transferred:       928600 bytes
  Requests per second:    22.18 [#/sec] (mean)
  Time per request:       45.086 [ms] (mean)
  Time per request:       45.086 [ms] (mean, across all concurrent requests)
  Transfer rate:          104.69 [Kbytes/sec] received

My sessions is stored on postgresql. It downs from 33.75rps to 22.18. It's a great performance penalty.

Other experiments: with file session backends speed ups Django to 32.50 rps (like old database session machinery) and with memcached (cache backend) it ups to 33.15rps, more or less like file backend also.

I think is a must to do a tunning and profiling here.

[Manuel Saelices]

Notes about number of queries send:

• The number of queries in old and the new session machinery is the same (2 SQL in my web):

  SELECT "django_session"."session_key","django_session"."session_data","django_session"."expire_date" FROM "django_session" WHERE ("django_session"."session_key" = 197396aae7fe82c8533f92a80955dd4c AND "django_session"."expire_date" > 2007-09-16 18:49:59.978334)
  SELECT "auth_user"."id","auth_user"."username","auth_user"."first_name","auth_user"."last_name","auth_user"."email","auth_user"."password","auth_user"."is_staff","auth_user"."is_active","auth_user"."is_superuser","auth_user"."last_login","auth_user"."date_joined" FROM "auth_user" WHERE ("auth_user"."id" = 1)
  

• Both file and cache backend speed up because the query number decreased to none.

[Manuel Saelices]

Fix for speedup sessions (like was before changeset [6333])

[Manuel Saelices]

Last patch already became django very fast!!! :-P

Any body can test it?

[Manuel Saelices]

Mmmmm... ticket is bad. I investigate again

[Manuel Saelices]

Forgot my prior comment. Patch works! :-D

[Fredrik Lundh <fredrik@…>]

Very fast as in? (from what I can tell, all you're saving is a property call. it shouldn't be *that* expensive).

Some additional micro-tuning for [6033]:

• move the import from process_request to module scope (settings won't change once we're up and running), or, if you want lazy loading, make it a module global and only import if None.

• there's tons of duplicate attribute accesses in the fast path; don't forget that locals are cheap in Python!

[Manuel Saelices]

I think ticket is fixed with this patch. The problem was:

• Before [6333]: session_key attribute of SessionWrapper object, was only a python attribute
• After [6333]: session_key attribute of SessionBase object (the same class after refactoring), is a python property, defined as:

  session_key = property(_get_session_key, _set_session_key)
  

Every access to self.session_key check is there is a session and creates one if not. But in code patched, i ask for session_key attribute, not for session_key in DB.

[Manuel Saelices]

Replying to Fredrik Lundh <fredrik@pythonware.com>:

Very fast as in? (from what I can tell, all you're saving is a property call. it shouldn't be *that* expensive).

Some additional micro-tuning for [6033]:

• move the import from process_request to module scope (settings won't change once we're up and running), or, if you want lazy loading, make it a module global and only import if None.

First I thought this was the problem, and I change this to a module scope. But it wasn't problem (it was slow afterall).

Once I fixed main problem I coulb test other questions, for improve again (if is it possible):

• Move __import__ statement
• Change a try...except for hasattr, in SessionBase._get_session

• there's tons of duplicate attribute accesses in the fast path; don't forget that locals are cheap in Python!

[Fredrik Lundh <fredrik@…>]

There's plenty of opportunities for micro-tuning here, I think, but let's get this patch in first. I fully agree with your analysis; boldly pushing this forward.

[Manuel Saelices]

Another patch for other minor performance improvements

[Manuel Saelices]

This is good patch for minor improvements. The other patch is other question.

[Manuel Saelices]

Sorry with this patch​, I didn't never should be upload. It's a big mistake. The last and good patch is [sessions_speedup_6364.2.diff this]

[Manuel Saelices]

Same patch than before but with correct indentation in process_request function

[Fredrik Lundh <fredrik@…>]

Ah, but now it got a lot harder to review ;-)

Nits:

• You don't need global in the method if you're only referring to the engine variable (it's only needed if you're assigning to it).

• Have you benchmarked the hasattr change?

[Manuel Saelices]

Replying to Fredrik Lundh <fredrik@pythonware.com>:

Ah, but now it got a lot harder to review ;-)

Nits:

• You don't need global in the method if you're only referring to the engine variable (it's only needed if you're assigning to it).

• Have you benchmarked the hasattr change?

I don't benchmarked this. Maybe try...except was better. But I found a very little improvement for changing location of __import__ statement.

[Jacob]

(In [6365]) Refs #5513: improved session performance after [6333]'s session refactoring. Thanks, msaelices.

[Jacob]

The session_key/_session_key bit was just simply a bug; I've checked that fix in. I'm a bit more suspicious of the speed improvements in the rest of the patch, so I'd want to do some benchmarks first.

FYI, I don't really trust benchmarks from ab (I've had lots of trouble getting reliable results without a huge margin of error). If you don't want to wait on me doing the benchmarks myself, you should use a better tool (I use ​Siege).

[Fredrik Lundh <fredrik@…>]

The import refactoring is a no-brainer (unless you want to support dynamic reconfiguration of the session engine, of course; I don't really see the point of that, but that's me).

The other one needs either proper benchmarking, or at least review by someone who knows what the use patterns really are.

(And as I mentioned earlier, there are lots of opportunities for micro-tuning here, but there's no need to keep this ticket open for that.)

[Malcolm Tredinnick]

Reading from the settings object during the import processing (of middleware.py) is something to avoid. So make engine=None initially and then only set it once, when required. ​Details here.

[Jacob]

After looking at this, I can't measure a difference between doing __import__ each time making a global var and doing the import if not engine:. If there is any difference, it's falling well within the margin of error in my test setup. So I'm going to mark this ticket fixed; please re-open if you find concrete performance improvements.