Opened 8 years ago
Closed 8 years ago
Last modified 8 years ago
#11868 closed enhancement (fixed)
|Reported by:||Ryan J Ollos||Owned by:||Ryan J Ollos|
|Cc:||Steffen Hoffmann, trac@…, leho@…||Branch:|
Performance improvement on systems with many thousands of authenticated users due to caching of
Removed the overridden method
The list of known users returned by
Environment.get_known_users should be cached.
Change History (18)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Proposed changes in log:rjollos.git:t11868, based on the branch from #11867.
I considered fixing this on 1.0-stable. Let me know any thoughts on that. I get the following results with attachment:test_get_known_users.py:
|Num Users||Time w/o caching||First retrieval with caching||Retrieval from cache|
|1k||7 ms||12 ms||< 1 ms|
|10k||70 ms||120 ms||< 1 ms|
|100k||800 - 900 ms||1200 ms||< 5 ms|
Not surprisingly it scales linearly with number of users.
by , 8 years ago
comment:3 by , 8 years ago
Looks pretty good.
Could you post the equivalent timing results without caching?
comment:4 by , 8 years ago
I've edited comment:2 to show results with and without caching.
comment:5 by , 8 years ago
comment:6 by , 8 years ago
comment:7 by , 8 years ago
Environment.db_query() returns a list of the result, not a generator.
>>> env = EnvironmentStub(default_data=True) >>> env.db_query("SELECT name, value FROM system") [(u'database_version', u'30'), (u'initial_database_version', u'30')]
I think we could return the value with no changes from
diff --git a/trac/env.py b/trac/env.py index 9d8150d..63b4829 100644
a b class Environment(Component, ComponentManager): 692 692 693 693 @cached 694 694 def _known_users(self): 695 return [(username, name, email) for username, name, email in 696 self.db_query(""" 695 return self.db_query(""" 697 696 SELECT DISTINCT s.sid, n.value, e.value 698 697 FROM session AS s 699 698 LEFT JOIN session_attribute AS n ON (n.sid=s.sid … … class Environment(Component, ComponentManager): 701 700 LEFT JOIN session_attribute AS e ON (e.sid=s.sid 702 701 AND e.authenticated=1 AND e.name = 'email') 703 702 WHERE s.authenticated=1 ORDER BY s.sid 704 """) ] 703 """) 705 704 706 705 def invalidate_known_users_cache(self): 707 706 """Clear the known_users cache."""
comment:8 by , 8 years ago
+1 for 1.0-stable
comment:9 by , 8 years ago
|Milestone:||1.1.3 → 1.0.3|
comment:10 by , 8 years ago
|API Changes:||modified (diff)|
|Status:||assigned → closed|
Thanks for the feedback. The changes have been committed,
- Removed the overridden method
- Added test cases for cached
Environment.get_known_users committed to 1.0-stable in  and merged to trunk in .
comment:11 by , 8 years ago
Another thing: I think we should call
Environment.invalidate_known_users_cache() when the user's name and email are changed in user preferences.
comment:12 by , 8 years ago
diff --git a/trac/web/session.py b/trac/web/session.py index d2b9eef..8692eb6 100644
a b class DetachedSession(dict): 137 137 # new ones. The last concurrent request to do so "wins". 138 138 139 139 if self._old != self: 140 141 140 142 if not items and not authenticated: 141 143 # No need to keep around empty unauthenticated sessions 142 144 db("DELETE FROM session WHERE sid=%s AND authenticated=0",
comment:13 by , 8 years ago
Thanks, I'll add tests and commit the change.
comment:14 by , 8 years ago
comment:15 by , 8 years ago
One additional change in [13590:13591].
comment:16 by , 8 years ago
|Release Notes:||modified (diff)|
comment:17 by , 8 years ago
Documentation improved in .
get_known_usersmethod added to
EnvironmentStubin  looks like an unnecessary override that makes it difficult to test the true behaviour of
get_known_users. It's straightforward to modify the tests to make the override unnecessary.
Removing the override is an API change; if anyone has concerns about that I can probably find a way to keep some backwards compatibility. A search of t-h.o reveals only one use of