Cache Environment.get_known_users

[Ryan J Ollos]

The list of known users returned by Environment.get_known_users should be cached.

[Ryan J Ollos]

The get_known_users method added to EnvironmentStub in [2799] 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 EnvironmentStub.known_users: ​browser:/tracformsplugin/trunk/0.11/tracforms/formdb.py@11137:418#L414.

[Ryan J Ollos]

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.

[Peter Suter]

Looks pretty good.

Could you post the equivalent timing results without caching?

[Ryan J Ollos]

I've edited comment:2 to show results with and without caching.

[Jun Omae]

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

trac/env.py

@@ -692 +692 @@
 
     @cached
     def _known_users(self):
-        return [(username, name, email) for username, name, email in
-                self.db_query("""
+        return self.db_query("""
                     SELECT DISTINCT s.sid, n.value, e.value
                     FROM session AS s
                      LEFT JOIN session_attribute AS n ON (n.sid=s.sid
@@ -701 +700 @@
                      LEFT JOIN session_attribute AS e ON (e.sid=s.sid
                       AND e.authenticated=1 AND e.name = 'email')
                     WHERE s.authenticated=1 ORDER BY s.sid
-                """)]
+                """)
 
     def invalidate_known_users_cache(self):
         """Clear the known_users cache."""

[anonymous]

+1 for 1.0-stable

[Ryan J Ollos]

Thanks for the feedback. The changes have been committed,

On trunk:

• Removed the overridden method get_known_users in EnvironmentStub in [13478].
• Added test cases for cached get_known_users in [13481]

Caching of Environment.get_known_users committed to 1.0-stable in [13479] and merged to trunk in [13480].

[Jun Omae]

Another thing: I think we should call Environment.invalidate_known_users_cache() when the user's name and email are changed in user preferences.

[Jun Omae]

trac/web/session.py

@@ -137 +137 @@
             # new ones. The last concurrent request to do so "wins".
 
             if self._old != self:
+                if self._old.get('name') != self.get('name') or \
+                        self._old.get('email') != self.get('email'):
+                    self.env.invalidate_known_users_cache()
                 if not items and not authenticated:
                     # No need to keep around empty unauthenticated sessions
                     db("DELETE FROM session WHERE sid=%s AND authenticated=0",

[Ryan J Ollos]

Thanks, I'll add tests and commit the change.

[Ryan J Ollos]

Fixed in [13488], merged in [13489]. Tests added on trunk in [13490].

[Ryan J Ollos]

One additional change in [13590:13591].

[Ryan J Ollos]

Documentation improved in [14259].