performance issue on PlonePAS (with patch > improve overall performance of Plone3.X)

[toutpt]

More you have users more your plone slow down.

The issue come from PlonePas. On each request (html , js, css , imgs) to build the user object it parse all users properties:

users=[ (user,data) for (user,data) in self._storage.items()

if self.testMemberData(data, criteria, exact_match)]

Take a look at the patch, it s easy to understand:

If the criteria is the userid => don't parse, just take it and return it.

comparative bench is available at:  http://jm406.free.fr/travail/plone/diff_01_connect_p_atch_bis_N4_vs_o_cache_no_admin-N2

lots of green !! enjoy !

[toutpt]

Patch to improve overall plone performance

[wichert]

This patch is not entirely correct: if you do enumerateUsers(id="nonexistingid") the plugin will now raise a KeyError on the data = self._storage[id] command. Please change that to check if the userid is present in self._storage, and if not return an empty list.

Even though PAS swallows exceptions normal usage should never raise them.

[toutpt]

New patch to not throw exeption on bad userid. remove return before then end of the code

[toutpt]

Replying to wichert:

This patch is not entirely correct: if you do enumerateUsers(id="nonexistingid") the plugin will now raise a KeyError on the data = self._storage[id] command. Please change that to check if the userid is present in self._storage, and if not return an empty list.

Even though PAS swallows exceptions normal usage should never raise them.

Sorry. I have fixed this in the second patch. I have also remove 'return' before the end of the code.

[vincentfretin]

I did a quick look at your patch, where is defined the user_id variable for the first else branch? You should have something like

user_id = id

no?

[naro]

I have tried it on my customized Plone instance, but enumerateUSers method is not called there. It is called on vanilla Plone from personal bar viewlet, document by line and workflow history, but not for each request.

[toutpt]

I think we have to first fix tests. I have about 6 failures when i launch tests of PlonePAS on plone3.2:

./bin/instance test -s Products.PlonePAS

Ran 55 tests with 6 failures and 0 errors in 6.749 seconds.

Next i will fix the patch, I don't like to code without tests.

[wichert]

I've commited a slightly cleaned up version of this patch to the 3.x and 4.0 branches and trunk.

[esteele]

Late to the show here, but doesn't this break the IUserEnumerationPlugin contract? ie "Allow querying users by ID, and searching for users." This specifically *disallows* querying for user by ID.

[vincentfretin]

The patch attached to this issue is not good, indeed this reimplementation is broken. PlonePAS 3.10 and 3.11 are broken. Please see the changelog of PlonePAS, and the last reimplementation in PlonePAS 3.12. I wrote a comment in the code "This plugin can't search for a user by id or login, because there is no such keys in the storage (data dict in the comprehensive list) If kw is empty or not, we continue the search."

You should compare the behaviour of PlonePAS 3.9 and 3.12.

[esteele]

Vincent, your changes ignore the fact that the userid is the key used in the plugin's _storage oobtree. I've banged my head against that one line of code numerous times over the past two weeks. We can't leave that in there...

[vincentfretin]

Hum, I'm not sure there. In PlonePAS 3.9, before the changes started, there was no support to search for a given id. In the first version of the patch, in 3.10 written by toutpt, there was support of it and it broke the listing of users in a certain way, I don't remember exactly. But I think the thing was that this enumerateUsers returned a group, yeah the _storage oobtree store users and groups!

So in 3.11, it was an attempt to revert to the same behaviour of 3.9, but I failed with the criteria thing. In fixed it in 3.12, for me this version has the same behaviour than 3.9.

As far as I can tell, until now, the use of mutable_properties.enumerateUsers with a given id was not used at all. We retrieve the fullname with user.getProperty('fullname') for example.

[vincentfretin]

I see the current implementation of branch 3.x is not the same as trunk.

I saw the issue #9435 and the associated changeset r94880 (which is not there as far as I can see :)

In trunk, there are still

        if id is not None:
            criteria["id"]=id
        if login is not None:
            criteria["login"]=login

Those lines are useless, because we don't have such key in data dict. I removed them in branch 3.x implementation.

In trunk we have:

            users=[ (user,data) for (user,data) in self._storage.items()
                        if self.testMemberData(data, criteria, exact_match) and not data.get('isGroup', False)]

And in branch 3.x, we have the same without "and not data.get('isGroup', False)". I think in most case, the testMemberData will return only users because we search 'fullname', and groups don't have 'fullname' in data dict. I don't know if there is a case where we can search with a given criteria key and both users and groups have this key is data dict. If it's the case, then the isGroup check is good.

In both implementation, 'login' is not used.

In trunk, I think the search by id is completely broken. In users, you only have data instead of [(key, data)] for it to work with the user_info comprehensive list.

I didn't tested, but maybe a reimplementation could be:

        plugin_id = self.getId()

        if id or login:
            user_id = id or login
            data = self._storage.get(user_id, None)
            if not data.get('isGroup', False):
                users = [(user_id, data)]
            else:
                return ()
        else:
            criteria = copy.copy(kw)

            users = [(user, data) for (user, data) in self._storage.items()
                     if not data.get('isGroup', False) and self.testMemberData(data, criteria, exact_match)]

        user_info = [{'id': self.prefix + user_id,
                      'login': user_id,
                      'title': data.get('fullname', user_id),
                      'description': data.get('fullname', user_id),
                      'email': data.get('email', ''),
                      'pluginid': plugin_id } for (user_id, data) in users]

        return tuple(user_info)