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

[11.x] Reduce the number of queries with Cache::many and Cache::putMany methods in the database driver #52209

Merged
merged 11 commits into from
Jul 22, 2024

Conversation

tonysm
Copy link
Contributor

@tonysm tonysm commented Jul 20, 2024

Changelog

  • Implements the many and putMany methods in the DatabaseStore cache driver. The previous implementation was doing a database query for each key, but now it does only 1 query for all (2 if it finds expired keys)

I was exploring fragment caching on my views... then found out the many method of the database cache driver was doing a database query for each cache key. The new implementation does only one query for all keys and then builds the results from what it finds. Additionally, it deletes the expired keys it finds in a single query as well.

The putMany had a similar story: it was doing a database query for each key. Now, it only does one upsert.

I made the get and put methods use the many and put ones behind the scenes so we can rely on the same implementation. This step is optional and we can revert the commit, if y'all don't want that.

@tonysm
Copy link
Contributor Author

tonysm commented Jul 20, 2024

The tests seem to be broken in 11.x, so I don't think it's because of these changes here.

@tonysm tonysm marked this pull request as draft July 21, 2024 12:05
@tonysm tonysm marked this pull request as ready for review July 21, 2024 21:01
*
* @return array
*/
public function many(array $keys)

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

and remove the dockblock @return pls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather keep the same method signature as the interface we're implementing.

Choose a reason for hiding this comment

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

If you are implementing an interface I agree, but I see this as a new function that is not in the interface.

Choose a reason for hiding this comment

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

@tonysm I apologize. I saw this commit 00dfdf4 and I will never ever open this subjet in the laravel community again.

* @param int $seconds
* @return bool
*/
public function putMany(array $values, $seconds)

Choose a reason for hiding this comment

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

analog pls.

Choose a reason for hiding this comment

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

/**
 * Store multiple items in the cache for a given number of seconds.
 */
public function putMany(array $values, int $seconds): bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, I'd rather keep the same method signature as the interface we're implementing. Plus, the type of the $seconds variable cannot be enforced because of the interface definition.

@taylorotwell taylorotwell merged commit 7363052 into laravel:11.x Jul 22, 2024
16 of 29 checks passed
@tonysm tonysm deleted the tm/database-cache-multi-get branch July 23, 2024 13:33
@francoism90
Copy link

@tonysm This seems interesting. Is the idea to store multiple models with the putMany method?

@tonysm
Copy link
Contributor Author

tonysm commented Jul 24, 2024

@francoism90 no, it's about caching view partials, and being able to retrieve all fragments in a single query is essential there. See the Rails Guide on this, it's a similar idea, but using my own Blade components or directives to achieve the same result.

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

Successfully merging this pull request may close these issues.

None yet

4 participants