Django

Code

Ticket #5513 (closed: fixed)

Opened 1 year ago

Last modified 1 year ago

Last session refactoring vastly down Django performance

Reported by: msaelices Assigned to: jacob
Milestone: Component: Contrib apps
Version: SVN Keywords: slowness performance
Cc: ferringb@gmail.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

sessions_speedup_6364.diff (0.5 kB) - added by msaelices on 09/16/07 13:18:05.
Fix for speedup sessions (like was before changeset [6333])
django_views_tests_r6360.diff (12.2 kB) - added by msaelices on 09/16/07 13:52:44.
Another patch for other minor performance improvements
sessions_speedup_6364.2.diff (1.6 kB) - added by msaelices on 09/16/07 13:54:04.
This is good patch for minor improvements. The other patch is other question.
sessions_speedup_6364.3.diff (1.7 kB) - added by msaelices on 09/16/07 14:03:43.
Same patch than before but with correct indentation in process_request function

Change History

09/16/07 11:51:39 changed by msaelices

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

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.

09/16/07 11:52:23 changed by msaelices

  • status changed from new to assigned.

09/16/07 11:52:36 changed by msaelices

  • keywords changed from slowness to slowness performance.

09/16/07 13:16:45 changed by anonymous

  • cc set to ferringb@gmail.com.

09/16/07 13:18:05 changed by msaelices

  • attachment sessions_speedup_6364.diff added.

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

09/16/07 13:18:16 changed by msaelices

  • has_patch set to 1.

09/16/07 13:19:08 changed by msaelices

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

Any body can test it?

09/16/07 13:25:09 changed by msaelices

Mmmmm... ticket is bad. I investigate again

09/16/07 13:29:28 changed by msaelices

Forgot my prior comment. Patch works! :-D

(follow-up: ↓ 11 ) 09/16/07 13:30:05 changed by 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.

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

09/16/07 13:39:39 changed 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.

(in reply to: ↑ 9 ) 09/16/07 13:43:19 changed 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!

09/16/07 13:47:17 changed by Fredrik Lundh <fredrik@pythonware.com>

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

09/16/07 13:49:21 changed by msaelices

  • stage changed from Ready for checkin to Accepted.

09/16/07 13:52:44 changed by msaelices

  • attachment django_views_tests_r6360.diff added.

Another patch for other minor performance improvements

09/16/07 13:54:04 changed by msaelices

  • attachment sessions_speedup_6364.2.diff added.

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

09/16/07 13:56:24 changed 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]

09/16/07 13:59:58 changed by msaelices

  • stage changed from Accepted to Ready for checkin.

09/16/07 14:03:43 changed by msaelices

  • attachment sessions_speedup_6364.3.diff added.

Same patch than before but with correct indentation in process_request function

(follow-up: ↓ 17 ) 09/16/07 14:10:45 changed by 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?

(in reply to: ↑ 16 ) 09/16/07 14:29:22 changed 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.

09/16/07 15:11:14 changed by jacob

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

09/16/07 15:14:35 changed 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).

09/16/07 15:20:00 changed by Fredrik Lundh <fredrik@pythonware.com>

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

09/16/07 18:26:32 changed 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.

09/19/07 12:02:18 changed by jacob

  • owner changed from msaelices to jacob.
  • status changed from assigned to new.

09/19/07 12:02:25 changed by jacob

  • status changed from new to assigned.

09/19/07 12:09:08 changed by jacob

  • status changed from assigned to closed.
  • resolution set to fixed.

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/Change #5513 (Last session refactoring vastly down Django performance)




Change Properties
Action