These are chat archives for ManageIQ/manageiq/performance

14th
Nov 2018
Dennis Metzger
@dmetzger57
Nov 14 2018 02:17
Thanks for the quick turn around on this investigation guys
Nick LaMuro
@NickLaMuro
Nov 14 2018 05:37

Threw this together for some more speed ups on the Hosts page:

ManageIQ/manageiq#18196

Will work on getting some better QA steps tomorrow

Joe Rafaniello
@jrafanie
Nov 14 2018 13:27
Present me thanks Past @NickLaMuro for fixing it and Present @NickLaMuro for finding it
Keenan Brock
@kbrock
Nov 14 2018 13:50
Present me thanks Present @NickLaMuro and awaits Future @NickLaMuro to remote the [WIP] from ManageIQ/manageiq#17475 and ManageIQ/manageiq#18196
Keenan Brock
@kbrock
Nov 14 2018 13:57
column virtual sql sort hidden cond comments
name sql sort
ipaddress sql
v_owning_cluster attr sql
v_total_vms attr sql
v_total_miq_templates attr sql
vmm_product sql
vmm_version sql
vmm_buildnumber sql
last_compliance_status attr ruby ManageIQ/manageiq#17475
ipmi_enabled attr ruby
last_scan_on attr ruby
region_description attr ruby
Keenan Brock
@kbrock
Nov 14 2018 14:08
@NickLaMuro would you add https://bugzilla.redhat.com/show_bug.cgi?id=1580982 to those PRs?
Keenan Brock
@kbrock
Nov 14 2018 14:21
I'm finishing up last_scan_on - just adding specs
have never found a good way for region_description.
At one time I had a branch for making region_id a virtual attribute but...
it didn't buy anything other than being able to see region_id / region_description as a sql column.
Just so long that we don't sort by region_id / region_description
Keenan Brock
@kbrock
Nov 14 2018 15:08
OMG. drift states sure are complicated because there is a large "blob" in the record and we're trying to avoid loading that value whenever possible :(
Keenan Brock
@kbrock
Nov 14 2018 15:15

There is a reselect kind of thing?
So order(:id).order(:name) == ORDER BY id, name
And order(:id).reorder(:name) == ORDER BY name

Wondering how to do this with select
virtual_delegate needs to clear out a select() specified on a relation

Keenan Brock
@kbrock
Nov 14 2018 15:43
answer ==> except(:select).select(:new_column)
Nick LaMuro
@NickLaMuro
Nov 14 2018 15:46
there is also unscope
Keenan Brock
@kbrock
Nov 14 2018 15:49
I'll revist. I was concerned that would undo too much. thnx
Nick LaMuro
@NickLaMuro
Nov 14 2018 15:50
sorry, that was just commenting about another method, but I think except is probably better since it is more targeted for your usecase
Keenan Brock
@kbrock
Nov 14 2018 15:50
I appreciate the reminder
Nick LaMuro
@NickLaMuro
Nov 14 2018 15:52

Regarding the comments from earlier...

awaits Future @NickLaMuro to ~remote~ remove the [WIP] from ManageIQ/manageiq#17475 and ManageIQ/manageiq#18196

I will be putting together some QA steps first before removing the [WIP]. I don't want the DB I am using to be the only way we can validate this

@NickLaMuro would you add https://bugzilla.redhat.com/show_bug.cgi?id=1580982 to those PRs?

Does this make sense? I figured the fix for the customer would be to move to 5.8.5 (where a fix exists), but then we would just have the PRs I have as a further performance improvement on top of it

Keenan Brock
@kbrock
Nov 14 2018 15:54
ok - so basically bug is already fixed in most recent build
Keenan Brock
@kbrock
Nov 14 2018 16:37
@NickLaMuro I added another to the ring for that view: ManageIQ/manageiq#18198
That PR was +5/-15, but the specs made it a +117/-15... I'm a little salty on that change :(
Nick LaMuro
@NickLaMuro
Nov 14 2018 16:54
I would hope that @jrafanie doesn’t count specs in his metrics...
Joe Rafaniello
@jrafanie
Nov 14 2018 16:56
@kbrock each line added is not the same... test lines are less maintanenance than code lines :wink:
Keenan Brock
@kbrock
Nov 14 2018 16:57
but they still show up on the stats in the github PR
oh cool:
host.vms.size does a count(*) - I thought it ran hosts.vms.to_a.size
but it actually runs: host.vms.count
BUT
hosts.vms.to_a ; hosts.vms.size == 1 query. while hosts.vms.to_a ; hosts.vms.count == 2 queries
I think I knew this at one time. recently I've been very count happy :(
Nick LaMuro
@NickLaMuro
Nov 14 2018 17:00
@kbrock Oh, I have a fix for that already
are you looking in the quadicon helper?
Keenan Brock
@kbrock
Nov 14 2018 17:00
looking at number_of helper
glad I went to "fix" some code and re-learned the size vs count thing
I think I use count way too much :(
Nick LaMuro
@NickLaMuro
Nov 14 2018 17:01
yeah, I think @Fryguy taught me that, and I think I have since forgotten about that too
Nick LaMuro
@NickLaMuro
Nov 14 2018 17:07
Keenan Brock
@kbrock
Nov 14 2018 17:07
thnx
This is nice. Did you see measurable improvement?
Nick LaMuro
@NickLaMuro
Nov 14 2018 17:09
yeah, it got rid of an N+1 on that page
Keenan Brock
@kbrock
Nov 14 2018 17:09
Also, number_of() caches the values.
I do wonder about changing v_total_vms to cache the values (kinda like vms caches values)
NICE
Nick LaMuro
@NickLaMuro
Nov 14 2018 17:10
for each quadicon, it has to fetch the total vms again, but with this, it just uses the attribute that is already part of the Host.yaml
Keenan Brock
@kbrock
Nov 14 2018 17:11
very nice
Nick LaMuro
@NickLaMuro
Nov 14 2018 17:24

fine (no patches)

/ems_infra/[EMS_ID]?display=hosts

ms queries query (ms) rows
14049 3193 2928.2 1355
11739 3195 1999.2 2746
21433 3192 1814.1 1354

master (with ManageIQ/manageiq#17473 and ManageIQ/manageiq#17474 )

(Note: Route changes on master due UI changes)

/ems_infra/report_data

ms queries query (ms) rows
12902 1895 1400.4 716
12729 1895 1723.8 716
14101 1894 1940.8 715
13687 1894 1673.2 715

master (with ManageIQ/manageiq#17475 )

/ems_infra/report_data

ms queries query (ms) rows
10774 1268 1736.2 716
14022 1267 1363 715
10239 1267 1180.0 715

master (with ManageIQ/manageiq-ui-classic#4923 )

/ems_infra/report_data

ms queries query (ms) rows
8966 641 692.4 716
7454 640 584.9 715
7074 640 589.4 715

master (with ManageIQ/manageiq#18196 and ManageIQ/manageiq-ui-classic#4924 )

/ems_infra/report_data

ms queries query (ms) rows
4907 13 153 715
4768 13 123.4 715
4688 13 172.3 715
6562 14 92.9 716

well, that formatting sucked... oh well...
The ems I was testing with in this case had around 620ish hosts, so each change was knocking out an N+1
Keenan Brock
@kbrock
Nov 14 2018 19:51
it looks like rails 5.0, 5.1, and 5.2 all store query parameters differently with respects to arel. So upgrades to 5.1 AND 5.2 have opportunity to trick us with virtual_attributes
I get confused with report_data vs the regular url