Code

Opened 7 years ago

Closed 7 years ago

#5513 closed (fixed)

Last session refactoring vastly down Django performance

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

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 msaelices 7 years ago.
Fix for speedup sessions (like was before changeset [6333])
django_views_tests_r6360.diff (12.2 KB) - added by msaelices 7 years ago.
Another patch for other minor performance improvements
sessions_speedup_6364.2.diff (1.6 KB) - added by msaelices 7 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 msaelices 7 years ago.
Same patch than before but with correct indentation in process_request function

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by msaelices

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 7 years ago by msaelices

  • Status changed from new to assigned

comment:3 Changed 7 years ago by msaelices

  • Keywords performance added

comment:4 Changed 7 years ago by anonymous

  • Cc ferringb@… added

Changed 7 years ago by msaelices

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

comment:5 Changed 7 years ago by msaelices

  • Has patch set

comment:6 Changed 7 years ago by msaelices

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

Any body can test it?

comment:7 Changed 7 years ago by msaelices

Mmmmm... ticket is bad. I investigate again

comment:8 Changed 7 years ago by msaelices

Forgot my prior comment. Patch works! :-D

comment:9 follow-up: Changed 7 years ago by 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!

comment:10 Changed 7 years ago by msaelices

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.

comment:11 in reply to: ↑ 9 Changed 7 years ago by msaelices

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 Changed 7 years ago by Fredrik Lundh <fredrik@…>

  • Triage Stage changed from Unreviewed to Ready 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 Changed 7 years ago by msaelices

  • Triage Stage changed from Ready for checkin to Accepted

Changed 7 years ago by msaelices

Another patch for other minor performance improvements

Changed 7 years ago by msaelices

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

comment:14 Changed 7 years ago by msaelices

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 Changed 7 years ago by msaelices

  • Triage Stage changed from Accepted to Ready for checkin

Changed 7 years ago by msaelices

Same patch than before but with correct indentation in process_request function

comment:16 follow-up: Changed 7 years ago by 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?

comment:17 in reply to: ↑ 16 Changed 7 years ago by msaelices

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 Changed 7 years ago by jacob

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

comment:19 Changed 7 years ago by 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).

comment:20 Changed 7 years ago by Fredrik Lundh <fredrik@…>

  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 7 years ago by mtredinnick

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 Changed 7 years ago by jacob

  • Owner changed from msaelices to jacob
  • Status changed from assigned to new

comment:23 Changed 7 years ago by jacob

  • Status changed from new to assigned

comment:24 Changed 7 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.