{"action":"created","issue":{"url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/issues/4447","repository_url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic","labels_url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/issues/4447/labels{/name}","comments_url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/issues/4447/comments","events_url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/issues/4447/events","html_url":"https://github.com/ManageIQ/manageiq-ui-classic/pull/4447","id":349298351,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA3NDQ5OTM0","number":4447,"title":"Stop N+1 with EmsDashboardService#recent_records","user":{"login":"NickLaMuro","id":314014,"node_id":"MDQ6VXNlcjMxNDAxNA==","avatar_url":"https://avatars0.githubusercontent.com/u/314014?v=4","gravatar_id":"","url":"https://api.github.com/users/NickLaMuro","html_url":"https://github.com/NickLaMuro","followers_url":"https://api.github.com/users/NickLaMuro/followers","following_url":"https://api.github.com/users/NickLaMuro/following{/other_user}","gists_url":"https://api.github.com/users/NickLaMuro/gists{/gist_id}","starred_url":"https://api.github.com/users/NickLaMuro/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NickLaMuro/subscriptions","organizations_url":"https://api.github.com/users/NickLaMuro/orgs","repos_url":"https://api.github.com/users/NickLaMuro/repos","events_url":"https://api.github.com/users/NickLaMuro/events{/privacy}","received_events_url":"https://api.github.com/users/NickLaMuro/received_events","type":"User","site_admin":false},"labels":[{"id":505074429,"node_id":"MDU6TGFiZWw1MDUwNzQ0Mjk=","url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/labels/bug","name":"bug","color":"ee0701","default":true},{"id":513017459,"node_id":"MDU6TGFiZWw1MTMwMTc0NTk=","url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/labels/performance","name":"performance","color":"e99695","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2018-08-09T21:31:42Z","updated_at":"2018-08-10T14:28:11Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/pulls/4447","html_url":"https://github.com/ManageIQ/manageiq-ui-classic/pull/4447","diff_url":"https://github.com/ManageIQ/manageiq-ui-classic/pull/4447.diff","patch_url":"https://github.com/ManageIQ/manageiq-ui-classic/pull/4447.patch"},"body":"The addition of `EmsDashboardService#recent_records` in https://github.com/ManageIQ/manageiq-ui-classic/pull/3427 causes an N+1, and seems to choke on the amazon provider because of the vast number of public images (`MiqTemplate` records) that are pulled in from a refresh.\r\n\r\nThis change provides a single query method for accomplishing the same task.  It is somewhat slow on a larger table (was testing with something around the size of 180k `MiqTemplate` records), but avoids any memory bloat entirely as we are only getting the counts.\r\n\r\n\r\nBenchmarks\r\n----------\r\n\r\nPrior to this change, the `/ems_cloud_dashboard/recent_images_data/:id` route would not even complete a request on my machine.  This was running against a small Amazon provider that had ~77k images (most of which were probably just public images from the Amazon EC2 marketplace), and just hit that N+1 endless (I stopped after the request ran for an hour and puma process was using 5+ gigs of RAM.\r\n\r\nAfter the change these are the results of a few refreshes against the Dashboard page:\r\n\r\n```\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 549ms\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 785ms\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 358ms\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 450ms\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 279ms\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 315ms\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 329ms\r\nGET    '/ems_cloud_dashboard/recent_images_data/4' 364ms\r\n```\r\n\r\n\r\nBackground\r\n----------\r\n\r\nThe code previously would create an initial query of the following:\r\n\r\n```SQL\r\nSELECT \"vms\".*\r\nFROM \"vms\"\r\nWHERE \"vms\".\"type\" IN ('MiqTemplate', ...)\r\n  AND \"vms\".\"template\"= 't'\r\n  AND (created_on > '2018-01-01 00:00:00.000000' and ems_id = 1)\r\n```\r\n\r\nThis means it fetches all of the `vms` table, which is 88 columns, for a given EMS and date range.  There is an addition of `.includes` in the original code as well, but I don't see how that was ever able to fire without an error as `@ems.id` would have most likely not worked, so the `@ems.blank?` check shouldn't have ever been triggered.\r\n\r\nFrom this is triggered by the `records.sort_by(&:created_on)` in the previous code, since `sort_by` is just inherited from `Enumerable` in Ruby's stdlib.  It then calls a `.uniq` on the records, and then iterates over each record (I am pretty sure this `uniq` does not do anything, but can't be sure).\r\n\r\nIn the code, we then see `strftime` applied on to the only column that was actually needed for every row returned (besides the id), and then a subsequent query is executed on each record to get count of every record that matches that date (forgetting to do the original sort by `ems.id` as well).\r\n\r\nAll to return a hash that looks like this:\r\n\r\n```ruby\r\n{\r\n  \"2018-01-01\" => 123,\r\n  \"2018-01-02\" => 234,\r\n  \"2018-01-03\" => 345,\r\n}\r\n```\r\n\r\n* * *\r\n\r\nThe code change does all of this, but in SQL, only returning from the DB an array of 2 element arrays of the same hash as above, and then converts that array into a hash.\r\n\r\nThis is done in a single query then, and only instantiates enough data to then generate the hash, but 0 `ActiveRecord` objects are necessary.  String formatting is also done in the postgres format, which is equivalent to Ruby's (and C's) `\"%Y-%m-%d\"` format.\r\n\r\n\r\nLinks\r\n-----\r\n\r\n* Introduced in https://github.com/ManageIQ/manageiq-ui-classic/pull/3427\r\n* Noticed when debugging https://bugzilla.redhat.com/show_bug.cgi?id=1610924\r\n\r\n\r\nSteps for Testing/QA\r\n--------------------\r\n\r\nI believe at a minimum, the steps done for https://github.com/ManageIQ/manageiq-ui-classic/pull/3427 should be reviewed and confirm that the counts provided on the dashboard are now (or still) accurate.  I have some questions about the previous code that has made me skeptical of the results shown had skewed numbers."},"comment":{"url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/issues/comments/412099119","html_url":"https://github.com/ManageIQ/manageiq-ui-classic/pull/4447#issuecomment-412099119","issue_url":"https://api.github.com/repos/ManageIQ/manageiq-ui-classic/issues/4447","id":412099119,"node_id":"MDEyOklzc3VlQ29tbWVudDQxMjA5OTExOQ==","user":{"login":"Fryguy","id":52120,"node_id":"MDQ6VXNlcjUyMTIw","avatar_url":"https://avatars3.githubusercontent.com/u/52120?v=4","gravatar_id":"","url":"https://api.github.com/users/Fryguy","html_url":"https://github.com/Fryguy","followers_url":"https://api.github.com/users/Fryguy/followers","following_url":"https://api.github.com/users/Fryguy/following{/other_user}","gists_url":"https://api.github.com/users/Fryguy/gists{/gist_id}","starred_url":"https://api.github.com/users/Fryguy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Fryguy/subscriptions","organizations_url":"https://api.github.com/users/Fryguy/orgs","repos_url":"https://api.github.com/users/Fryguy/repos","events_url":"https://api.github.com/users/Fryguy/events{/privacy}","received_events_url":"https://api.github.com/users/Fryguy/received_events","type":"User","site_admin":false},"created_at":"2018-08-10T14:28:11Z","updated_at":"2018-08-10T14:28:11Z","author_association":"MEMBER","body":"Additionally if you move this to a scope, prefer `.size` over `.count`.  `.count` *always* goes back to the DB, but `.size` will use the local cache if it's available.  In this particular PR it won't make a difference because you are always building a new scope, but if moved to a generic solution, `.size` tends to be better.\r\n\r\n```ruby\r\n[1] pry(main)> q = Vm.all; nil\r\n[2] pry(main)> q.size\r\n   (0.8ms)  SELECT COUNT(*) FROM \"vms\" ...\r\n=> 259\r\n[3] pry(main)> q.size\r\n   (0.8ms)  SELECT COUNT(*) FROM \"vms\" ...\r\n=> 259\r\n[4] pry(main)> q.to_a; nil\r\n  Vm Load (3.0ms)  SELECT \"vms\".* FROM \"vms\" ...\r\n  Vm Inst Including Associations (16.9ms - 259rows)\r\n=> nil\r\n[5] pry(main)> q.size\r\n=> 259\r\n[6] pry(main)> q.size\r\n=> 259\r\n```"}}
{"actor":{"display_login":"Fryguy"}}