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

Fix memory-hog query in AssetCountForSidebar middleware #14711

Merged
merged 1 commit into from May 11, 2024

Conversation

jerm
Copy link
Collaborator

@jerm jerm commented May 10, 2024

https://github.com/snipe/snipe-it/pull/14702/files introduced a bug
where instead of doing a quick select count(*) of assets, it did a select * of
assets, moving the count from the database to the PHP process.

This caused OOM issues in memory-constrained environments with lots of
assets, and also presented a speed issue even when memory limited were
increased.

Additionally, given this populates the sidebar, this was likely an issue
on every page load that included the sidebar.

The fix is simply removing the all()->, ending up with Asset::count(),
which yields the desired select count(*) DB query.

https://github.com/snipe/snipe-it/pull/14702/files introduced a bug
where instead of doing a quick `select count(*)` of assets, it did a `select *` of
assets, moving the count from the database to the PHP process.

This caused OOM issues in memory-constrained environments with lots of
assets, and also presented a speed issue even when memory limited were
increased.

Additionally, given this populates the sidebar, this was likely an issue
on every page load that included the sidebar.

The fix is simply removing the `all()->`, ending up with Asset::count(),
which yields the desired `select count(*)` DB query.
@jerm jerm requested a review from snipe as a code owner May 10, 2024 20:01
Copy link

what-the-diff bot commented May 10, 2024

PR Summary

  • Optimized Asset Count Calculation
    The modification in the AssetCountForSidebar.php file changes our method of counting the total number of assets. Previously, we were getting all assets then counting them, which was less efficient. The new method simply counts the assets directly, enhancing overall performance of the sidebar where this total count is displayed.

@snipe snipe merged commit 2decc3d into develop May 11, 2024
8 checks passed
@snipe snipe deleted the jerm/fix-all-query-in-sidebar-middleware branch May 11, 2024 00:35
@jerm jerm mentioned this pull request May 14, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants