Opened 17 years ago

Closed 17 years ago

#5513 closed (fixed)

Last session refactoring vastly down Django performance

Reported by: Manuel Saelices Owned by: Jacob
Component: Contrib apps Version: dev
Severity: Keywords: slowness performance
Cc: ferringb@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (4)

sessions_speedup_6364.diff (546 bytes ) - added by Manuel Saelices 17 years ago.
Fix for speedup sessions (like was before changeset [6333])
django_views_tests_r6360.diff (12.2 KB ) - added by Manuel Saelices 17 years ago.
Another patch for other minor performance improvements
sessions_speedup_6364.2.diff (1.6 KB ) - added by Manuel Saelices 17 years ago.
This is good patch for minor improvements. The other patch is other question.
sessions_speedup_6364.3.diff (1.7 KB ) - added by Manuel Saelices 17 years ago.
Same patch than before but with correct indentation in process_request function

Download all attachments as: .zip

Change History (28)

comment:1 by Manuel Saelices, 17 years ago

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.

comment:2 by Manuel Saelices, 17 years ago

Status: newassigned

comment:3 by Manuel Saelices, 17 years ago

Keywords: performance added

comment:4 by anonymous, 17 years ago

Cc: ferringb@… added

by Manuel Saelices, 17 years ago

Attachment: sessions_speedup_6364.diff added

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

comment:5 by Manuel Saelices, 17 years ago

Has patch: set

comment:6 by Manuel Saelices, 17 years ago

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

Any body can test it?

comment:7 by Manuel Saelices, 17 years ago

Mmmmm... ticket is bad. I investigate again

comment:8 by Manuel Saelices, 17 years ago

Forgot my prior comment. Patch works! :-D

comment:9 by Fredrik Lundh <fredrik@…>, 17 years ago

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!

comment:10 by Manuel Saelices, 17 years ago

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.

in reply to:  9 comment:11 by Manuel Saelices, 17 years ago

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!

comment:12 by Fredrik Lundh <fredrik@…>, 17 years ago

Triage Stage: UnreviewedReady for checkin

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.

comment:13 by Manuel Saelices, 17 years ago

Triage Stage: Ready for checkinAccepted

by Manuel Saelices, 17 years ago

Another patch for other minor performance improvements

by Manuel Saelices, 17 years ago

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

comment:14 by Manuel Saelices, 17 years ago

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]

comment:15 by Manuel Saelices, 17 years ago

Triage Stage: AcceptedReady for checkin

by Manuel Saelices, 17 years ago

Same patch than before but with correct indentation in process_request function

comment:16 by Fredrik Lundh <fredrik@…>, 17 years ago

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?

in reply to:  16 comment:17 by Manuel Saelices, 17 years ago

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.

comment:18 by Jacob, 17 years ago

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

comment:19 by Jacob, 17 years ago

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).

comment:20 by Fredrik Lundh <fredrik@…>, 17 years ago

Triage Stage: Ready for checkinAccepted

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.)

comment:21 by Malcolm Tredinnick, 17 years ago

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.

comment:22 by Jacob, 17 years ago

Owner: changed from Manuel Saelices to Jacob
Status: assignednew

comment:23 by Jacob, 17 years ago

Status: newassigned

comment:24 by Jacob, 17 years ago

Resolution: fixed
Status: assignedclosed

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.

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