Laravel-permission: L5.3: Performance issues

[fabricecw]

We're running a Laravel 5.3 application with laravel-permission. When updating the package from 1.12.0 to 1.16.1 we have massive performance issues.

Request duration (measured with Debugbar)
1.12.0: 400ms
1.16.1: 8300ms

Queries:
In both versions the same duration.

Are there any known problems with L5.3?

Thanks and regards,

[drbyte]

@fabricecw I'm not currently using L5.3 anywhere, but in looking at the code diffs perhaps something with the Gate changes in the PermissionRegistrar may be contributing to the problem? I don't recall those changes being "breaking" between Laravel versions, but you might want to look at what's happening there?

https://github.com/spatie/laravel-permission/compare/1.12.0...1.16.1

[fabricecw]

@drbyte You're right. It seems that the changes from issue #505 is the bottleneck. If I replace the registerPermissions() method in the PermissionRegistrar with the code from 1.12.0 everything works fine. But if I'm honest, I don't fully understand the logic of the new code. Maybe someone can explain the changes to me :-)

[drbyte]

You said:

Request duration (measured with Debugbar)
1.12.0: 400ms
1.16.1: 8300ms
Queries:
In both versions the same duration.

Question: Did you note whether the number-of-queries is much different too?

[fabricecw]

The number of executed queries are in both versions the same. The permission queries also looks the same.

[drbyte]

That's interesting. I guess the cache insulates against any variation in permission queries.

Before #505 the registration of permissions was done by loading all permissions from db/cache and then register a callback for each one into the Gate::$abilities array. When checking allow/deny that would lookup whether an ability was defined, and run it ... which ultimately would run $user->hasPermissionTo($permission) for the requested ability.

The change made in #505 (and also #538) registers (not an array of all permissions, but) a single callback for the Authenticatable contract. Thus when an allow/deny request is made, it fires that callback with the passed arguments and does the lookup ... which also ultimately runs $user->hasPermissionTo($permission) for the requested ability.

Since the change uses a single callback instead of adding every permission to the call stack I would have expected it to be more performant, not less.

[drbyte]

@fabricecw Is this still an issue?
How long is this app likely to be on L5.3?

I have an app using v1.x of this package, but it's not having noticeable performance issues. Granted, it's running L5.6, not L5.3.
(Edit: actually, there are a lot of gate-related queries, but no notable change in measured speed)

[fabricecw]

@drbyte We have planned to update the app at least to L5.5 in a few weeks. I will let you know if the performance is better then. Thank you for taking the time.

[drbyte]

Great. Closing this issue for now.

[fabricecw]

We updated our application to L5.7 and also upgraded from V1.13 to the V2.28 version of laravel-permission. But unfortunately, the performance issue still exists.

If I replace the following code in the PermissionRegistrar.php:

public function registerPermissions(): bool
    {
        $this->gate->before(function (Authorizable $user, string $ability) {
            try {
                if (method_exists($user, 'hasPermissionTo')) {
                    return $user->hasPermissionTo($ability) ?: null;
                }
            } catch (PermissionDoesNotExist $e) {
            }
        });

        return true;
    }

with the following code:

public function registerPermissions()
    {
        $this->getPermissions()->map(function ($permission) {
            $this->gate->define($permission->name, function ($user) use ($permission) {
                return $user->hasPermissionTo($permission);
            });
        });
    }

we don't have any problems. Depending on the amount of orders displayed in the view, Laravel loads between 80 and 200 gates. The view checks each order with the @can() Blade function. The user's permission are loaded through a role (user has one role). All permissions are assigned to roles.

If I use the default code, 40 gates lead to loading time of ~1500MS. With 80 gates, the loading time increases up to ~2500MS.

If I use the registerPermissions() from V1.13, 80 gates needs a loading time of ~120MS, 200 gates ~140MS.

[drbyte]

@fabricecw,
It makes sense that it would do fewer calls, since it only reads permissions once, and sets a Gate rule for each of them. The gate->before approach is different in that it hits the gate individually for each call, which is less performant.
I'm looking into going back to the define approach. However, directly using the code you quoted (from v1) most of the v2 tests to break due to db chicken/egg presence problems.
Even wrapping it inside a gate->before call like was proposed in #480 doesn't fully fix it, as 6+ tests fail (all the ones calling can).
I don't think it's related only to guards, but that was my first suspect.

A PR is welcome.

[fabricecw]

I tested it again in my code and wrapped the old approach into the before callback.

/**
     * Register the permission check method on the gate.
     *
     * @return bool
     */
    public function registerPermissions(): bool
    {
        $this->gate->before(function() {
            $this->loadPermissionsToGate();
        });

        return true;
    }

    /**
     * Load the permissions to the gate.
     *
     * @return bool
     */
    public function loadPermissionsToGate(): bool
    {
        $this->getPermissions()->map(function ($permission) {
            $this->gate->define($permission->name, function (Authorizable $user) use ($permission) {
                try {
                    if (method_exists($user, 'hasPermissionTo')) {
                        return $user->hasPermissionTo($permission) ?: null;
                    }
                } catch (PermissionDoesNotExist $e) {
                }

                return true;
            });
        });

        return true;
    }

Execution time: 2980ms

If I call the define directly:
/**
     * Register the permission check method on the gate.
     *
     * @return bool
     */
    public function registerPermissions(): bool
    {
        $this->loadPermissionsToGate();

        return true;
    }

    /**
     * Load the permissions to the gate.
     *
     * @return bool
     */
    public function loadPermissionsToGate(): bool
    {
        $this->getPermissions()->map(function ($permission) {
            $this->gate->define($permission->name, function (Authorizable $user) use ($permission) {
                try {
                    if (method_exists($user, 'hasPermissionTo')) {
                        return $user->hasPermissionTo($permission) ?: null;
                    }
                } catch (PermissionDoesNotExist $e) {
                }

                return true;
            });
        });

        return true;
    }

Execution time: 105ms

It doesn't make sense for me, because as I understand it, Gate::before only collect callback functions.

[kamov]

I guess why they use gate::before and not define.

I think that gate::before is called each time can() is called or any other check if ability.

[lkreher]

Why can't we combine both approaches like this?

    protected $permissionsLoaded = false;

    /**
     * Register the permission check method on the gate.
     *
     * @return bool
     */
    public function registerPermissions(): bool
    {
        $this->gate->before(function () {
            if (!$this->permissionsLoaded) {
                $this->loadPermissionsToGate();
                $this->permissionsLoaded = true;
            }
        });

        return true;
    }

    /**
     * Load the permissions to the gate.
     *
     * @return bool
     */
    public function loadPermissionsToGate(): bool
    {
        $this->getPermissions()->map(function ($permission) {
            $this->gate->define($permission->name, function (Authorizable $user) use ($permission) {
                try {
                    if (method_exists($user, 'hasPermissionTo')) {
                        return $user->hasPermissionTo($permission);
                    }
                } catch (PermissionDoesNotExist $e) {
                }
            });
        });

        return true;
    }

We don't want to call getPermissions() on register, because that action requires a DB-connection. But also we don't want to call getPermissions() before every gate check, since it introduces a significant performance hit (in my testing environment about 6ms per can())

This combination avoids both problems and shouldn't break anything.

[drbyte]

@lkreher wanna help debug the failing tests that this introduces? Basically all the can() calls fail.

[fabricecw]

@drbyte I‘ll take a look at them.

[lkreher]

@lkreher wanna help debug the failing tests that this introduces? Basically all the can() calls fail.

I'd love to, but I'm new to package development, can you give me a quickstart-link on how to setup a dev version of this package?

I think the problem might be the very first can() call per script not being handled correctly, since the gates are only just being defined at that moment.

[drbyte]

https://github.com/spatie/laravel-permission/releases/tag/2.35.0 contains the excellent fixes from @matthewgoslett which should help with these performance issues.