Slow updates to lists (edit_list_conf cache not working)

[dpc22]

Version

6.2.60

Installation method

RHEL 7 rpm

Expected behaviour

Updates to list configuration via wwsympa "Admin -> Edit List Config" (edit_list_request) should take less than a second to complete.

Actual behaviour

Updates take 5 seconds to complete. (Dedicated test machine. Zero load).

Additional information

If I enable logging, almost all of the time is spent in:

/usr/share/sympa/lib/Sympa/List/Config.pm

sub get_schema {
...
        foreach my $pname (CORE::keys %{$pinfo || {}}) {
            $self->_get_schema_apply_privilege($pinfo->{$pname}, [$pname],
                $user, undef);
        }

Namely _get_schema_apply_privilege:

    my $priv = $list->may_edit(join('.', @{$pnames || []}), $user);

which calls /usr/share/sympa/lib/Sympa/List.pm ::

sub may_edit {
...
   my $edit_list_conf =
           $self->_cache_get('edit_list_conf')
        || $self->_cache_put('edit_list_conf', $self->_load_edit_list_conf)
        || {};

Testing reveals that the cache is never actually used:

sub _cache_get {
    my $self = shift;
    my $type = shift;

    my $lasttime = $self->{_mtime}{$type};
    my $mtime;
    if ($type eq 'total' or $type eq 'is_list_member') {
        $mtime = $self->_cache_read_expiry('member');
    } else {
        $mtime = $self->_cache_read_expiry($type);
    }
    $self->{_mtime}{$type} = $mtime;

    return undef unless defined $lasttime and defined $mtime;
    return undef if $lasttime <= $mtime;
    return $self->{_cached}{$type};
}

$self->-cache_get() is comparing the mtime on the relevant file with the mtime of the same file on the previous call.
Consequently ($lasttime <= $mtime) in the following is always true:

    return undef if $lasttime <= $mtime;

unless the file is updated in the middle of an get_schema() call (which should almost never happen, and is the reverse of the desired behaviour for a cache).

I attach a patch which seems closer to the intended behaviour, including a debug statement:

List.pm.patch.txt

This is much faster

[ikedas]

Thanks @dpc22, I review your patch and will merge it if possible.

BTW cache feature is proposed to be removed in the future (not in short term): See #584 for details.

[dpc22]

I attach a different patch:

dpc_get_schema_cache.patch.txt

This calls $self->_load_edit_list_conf() once. It then passes the value in as parameter to may_edit() using $options{_edit_list_conf_cache}: that seemed safer than adding yet another unnamed parameter to may_edit()

I do recommend this: it makes Sympa feel much more responsive, especially given two calls to get_schema() on every update to the list.

I would also recommend just throwing the existing cache stuff away right now: it only returns cached values when the underlying file changes, which is the opposite of the desired behaviour for a cache and may be causing bad side effects.

[dpc22]

By the way: does #937 make a substantial difference to do_lists speed?

This is a concern for me as we will eventually have 7000 lists on one Sympa robot. I have currently just removed link to the "Index of lists" page so that robots (and my users) can't find it easier.

[ikedas]

I could be wrong, but your second patch looks not reloading edit_list.conf even when it had been updated.

I don't recommend improving the code of cache mechanism: In our experiences, it is difficult to write stablly maintainable code using "cache" or similar things, while not so much can be gained from such improvement.

In the first place, is it worth constantly checking for updates of edit_list.conf? I suppose that this file only needs to be read once when $list->load() is called.

[dpc22]

edit_list.conf is loaded at the start of each get_schema() call and then the same value is used for each call to may_edit(). If edit_list.conf changes it will be picked up on the next get_schema() call.

I'm pretty certain that this is safe. In fact I would argue that it is safer that the current approach when edit_list.conf is updated in the middle of the 5 second window where $self->_load_edit_list_conf() is being hammered.

I agree that the existing generic cache code should go. This is dealing with one specific case. It definitely makes Sympa feel more responsive, and we aren't spinning at 100% user CPU for 5 seconds every time one attribute on a list changes.

I'm happy to have a go at a third patch if you prefer that approach, although I think that this would be a larger patch and rather more invasive.

may_edit() is called several hundred times in rapid succession, to test every possible attribute of a list even if only one attribute is updated. I guess that an alternative approach would be to work out which attributes have changed and only run may_edit() for those parameters. However that would be rather more work.

[ikedas]

edit_list.conf is loaded at the start of each get_schema() call and then the same value is used for each call to may_edit(). If edit_list.conf changes it will be picked up on the next get_schema() call.

I'm pretty certain that this is safe. In fact I would argue that it is safer that the current approach when edit_list.conf is updated in the middle of the 5 second window where $self->_load_edit_list_conf() is being hammered.

I understand.

I agree that the existing generic cache code should go. This is dealing with one specific case. It definitely makes Sympa feel more responsive, and we aren't spinning at 100% user CPU for 5 seconds every time one attribute on a list changes.

Cache code is proposed to be removed. See also #584.

I'm happy to have a go at a third patch if you prefer that approach, although I think that this would be a larger patch and rather more invasive.

may_edit() is called several hundred times in rapid succession, to test every possible attribute of a list even if only one attribute is updated. I guess that an alternative approach would be to work out which attributes have changed and only run may_edit() for those parameters. However that would be rather more work.

Though I've not confirmed if it will work,

(The patch was revised. See the PR below.)

[ikedas]

@dpc22, if possible, could you please check the patch in PR above?

[dpc22]

Will do. May not be for a day or two, but thank you.

[dpc22]

Okay, that certain seems to work, the top level $self->{_edit_list} is much nicer idea.

I will push that onto our main Sympa server later today.

[ikedas]

@dpc22, this issue is closed for now. If you would notice anything, please feel free to reopen this issue.
Thanks for reporting bug!