Edgewall Software
Modify

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#11868 closed enhancement (fixed)

Cache Environment.get_known_users

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.3
Component: general Version:
Severity: normal Keywords: environment
Cc: Steffen Hoffmann, trac@…, leho@… Branch:
Release Notes:

Performance improvement on systems with many thousands of authenticated users due to caching of Environment.get_known_users().

API Changes:

Removed the overridden method get_known_users in EnvironmentStub that returned users from the attribute known_users (1.1.3).

Internal Changes:

Description

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

Attachments (1)

test_get_known_users.py (1.6 KB ) - added by Ryan J Ollos 9 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Ryan J Ollos, 9 years ago

Cc: Steffen Hoffmann added

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.

Last edited 9 years ago by Ryan J Ollos (previous) (diff)

comment:2 by Ryan J Ollos, 9 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Status: newassigned

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.

Last edited 9 years ago by Ryan J Ollos (previous) (diff)

by Ryan J Ollos, 9 years ago

Attachment: test_get_known_users.py added

comment:3 by Peter Suter, 9 years ago

Looks pretty good.

Could you post the equivalent timing results without caching?

comment:4 by Ryan J Ollos, 9 years ago

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

comment:5 by trac@…, 9 years ago

Cc: trac@… added

comment:6 by lkraav <leho@…>, 9 years ago

Cc: leho@… added

comment:7 by Jun Omae, 9 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 _known_users().

  • trac/env.py

    diff --git a/trac/env.py b/trac/env.py
    index 9d8150d..63b4829 100644
    a b class Environment(Component, ComponentManager):  
    692692
    693693    @cached
    694694    def _known_users(self):
    695         return [(username, name, email) for username, name, email in
    696                 self.db_query("""
     695        return self.db_query("""
    697696                    SELECT DISTINCT s.sid, n.value, e.value
    698697                    FROM session AS s
    699698                     LEFT JOIN session_attribute AS n ON (n.sid=s.sid
    class Environment(Component, ComponentManager):  
    701700                     LEFT JOIN session_attribute AS e ON (e.sid=s.sid
    702701                      AND e.authenticated=1 AND e.name = 'email')
    703702                    WHERE s.authenticated=1 ORDER BY s.sid
    704                 """)]
     703                """)
    705704
    706705    def invalidate_known_users_cache(self):
    707706        """Clear the known_users cache."""

comment:8 by anonymous, 9 years ago

+1 for 1.0-stable

comment:9 by Ryan J Ollos, 9 years ago

Milestone: 1.1.31.0.3

comment:10 by Ryan J Ollos, 9 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

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

comment:11 by Jun Omae, 9 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 Jun Omae, 9 years ago

  • trac/web/session.py

    diff --git a/trac/web/session.py b/trac/web/session.py
    index d2b9eef..7a3593e 100644
    a b class DetachedSession(dict):  
    137137            # new ones. The last concurrent request to do so "wins".
    138138
    139139            if self._old != self:
     140                if self._old.get('name') != self.get('name') or \
     141                        self._old.get('email') != self.get('email'):
     142                    self.env.invalidate_known_users_cache()
    140143                if not items and not authenticated:
    141144                    # No need to keep around empty unauthenticated sessions
    142145                    db("DELETE FROM session WHERE sid=%s AND authenticated=0",
Last edited 9 years ago by Jun Omae (previous) (diff)

comment:13 by Ryan J Ollos, 9 years ago

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

comment:14 by Ryan J Ollos, 9 years ago

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

comment:15 by Ryan J Ollos, 9 years ago

One additional change in [13590:13591].

Last edited 9 years ago by Ryan J Ollos (previous) (diff)

comment:16 by Ryan J Ollos, 9 years ago

Release Notes: modified (diff)

comment:17 by Ryan J Ollos, 9 years ago

Documentation improved in [14259].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.