Improve database performance by removing duplicated queries and using eager loading #3357
Conversation
|
Congratulations on opening your first Pull Request, this is a momentous day for you and us! ✨
|
AdrienPoupa
changed the title
There was a problem hiding this comment.
This is a great change @AdrienPoupa, thank you very much!
Just a couple of changes, please.
| */ | ||
| public function enabled_components_lowest() | ||
| { | ||
| if (is_bool($this->enabledComponentsLowest)) { |
There was a problem hiding this comment.
Why are we checking is_bool if the variable is declared as false by default?
| @@ -248,9 +250,11 @@ public function latest_icon() | |||
| */ | |||
| public function latest() | |||
| { | |||
| if ($update = $this->wrappedObject->updates()->orderBy('created_at', 'desc')->first()) { | |||
| return $update; | |||
| if (is_bool($this->latest)) { | |||
There was a problem hiding this comment.
I think we can just say if ($this->latest).
|
Ping @AdrienPoupa |
|
Hi, Thanks for the review. I chose to use a boolean because by default, if a var is not declared in PHP, it's a null. If there is no hit in the database, Eloquent returns a null as well. Thus, we need a way to know if we queried the database at least once; if we were checking a null and Eloquent returned a null we would be checking over and over again... By using is_bool, we know for sure that for the current request, if the attribute is a boolean, the query was not executed yet. After the execution, it can be set either to its actual value, or null if nothing was found in the database. |
|
Hooray! Your first Pull Request was merged, here's to many more 🚀 |
|
Awesome, thank you! |
This PR eliminates some unnecessary queries:
The
DashboardComposerwas executed as many times as there were dashboard views rendered, resulting in 5 additional queries for each execution when one was enough. To ensure that it was run only once, I used this method.Also, for some reason, the retrieval of the setting
enable_external_dependencieswas done usingsettingand notCache::, thus a query to the database was done whenever an avatar was displayed.In
IncidentController, eager loading the user is useful because the dashboard.incident.index view queries$incident->usertwice.In ComponentController, eager loading the group is useful because it is queried twice per component. On my test instance that has 12 components, the dashboard/components goes from 35 queries, 5.3Mb and 60ms to 10 queries, 4.8Mb and 46ms
Finally, on the homepage, by adding eager loadings to
$allIncidentsin theStatusPageControllerand creating attributes that act like a cache, on a large database, I have reduced the number of queries from 539 to 118, the memory consumption from 18Mb to 8Mb and the loading time from 530ms to 240ms.