Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache user object when checking existance #412

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

CarlSchwan
Copy link
Member

In server:lib/private/User/Manager.php::get we always first check for
the existance of the user and then try to fetch it. Caching the user
should save us one call to the DB on each request for a OIDC user.

In server:lib/private/User/Manager.php::get we always first check for
the existance of the user and then try to fetch it. Caching the user
should save us one call to the DB on each request for a OIDC user.

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

@juliusknorr juliusknorr merged commit b2df41b into master Apr 5, 2022
@juliusknorr juliusknorr deleted the perf/cache-user-when-checking-existance branch April 5, 2022 13:36
@juliusknorr juliusknorr mentioned this pull request Apr 11, 2022
13 tasks
@brederle
Copy link

Hey Carl,

I don't think the patch is really optimal as it now calls getUser twice in userExists. I would put the caching at the
end of getUser like:

	$qb->select('*')
			->from($this->getTableName())
			->where(
				$qb->expr()->eq('user_id', $qb->createNamedParameter($uid))
			);

        $dbuser = $this->findEntity($qb);
        $this->userCache->set($uid, $dbuser);
	return $dbuser;

@CarlSchwan
Copy link
Member Author

Hey Carl,

I don't think the patch is really optimal as it now calls getUser twice in userExists. I would put the caching at the end of getUser like:

	$qb->select('*')
			->from($this->getTableName())
			->where(
				$qb->expr()->eq('user_id', $qb->createNamedParameter($uid))
			);

        $dbuser = $this->findEntity($qb);
        $this->userCache->set($uid, $dbuser);
	return $dbuser;

getUser in userExists didn't get called twice because the old getUser line was removed. But it indeed make even more sense to cache in getUser I created a PR for that in #435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants