These are chat archives for ManageIQ/manageiq/performance

22nd
Jun 2016
Keenan Brock
@kbrock
Jun 22 2016 03:31
@jrafanie I hope #9263 shows why I like virtual_totals.
Once you get to a big provider, the numbers are drastic
Haven't had the chance to really tune all the types of virtual totals, but it has a lot of promise
Keenan Brock
@kbrock
Jun 22 2016 13:08
@Fryguy ask and ye shall receive: ManageIQ/manageiq#9350
^ that allows us to bring back totals in a single query and avoid all the N+1 entries
Jason Frey
@Fryguy
Jun 22 2016 13:51
yeah, that's what I figured
so then I'd need to find a way to change the TreeBuilder to use that new computed column
Keenan Brock
@kbrock
Jun 22 2016 13:53
using computed columns there only work for has_many right now
again - when there is a little free time...
Keenan Brock
@kbrock
Jun 22 2016 15:25
Any suggestions how to avoid using attributes in the following:
VmOrTemplate.select("(select 'a') as a").first.attributes["a"]
this is as simplified as I could make it. that inner select is much more complicated
Nick LaMuro
@NickLaMuro
Jun 22 2016 15:27
morbid curiosity, what are we avoiding by not calling attributes?
or trying to avoid
Joe Rafaniello
@jrafanie
Jun 22 2016 15:27
I don't know.. .pluck(:a).first
I'll try pluck
Joe Rafaniello
@jrafanie
Jun 22 2016 15:28
yeah, I don't know if it works with select
I remember there were some changes with pluck from 4.0-4.2, can't remember details
Keenan Brock
@kbrock
Jun 22 2016 15:29
yea - it is better
and pluck(:a) is quicker than pluck(&:a)
man - so I convert an n+1 to a 1
and jason wants to avoid duplicating the attributes arary :(
Joe Rafaniello
@jrafanie
Jun 22 2016 15:30
high level, why?
n+1's are sometimes better than preloading stuff you don't use/need
we've already seen that the N+1 tradeoff for memory is a delicate balance with ruby 2.1+ gen gc
Keenan Brock
@kbrock
Jun 22 2016 15:32
This message was deleted
downloading all data to avoid n+1 is bad
that is agreed
but I'm only downloading 1 number
aah
Joe Rafaniello
@jrafanie
Jun 22 2016 15:33
So, all I'm asking is measure before/after to make sure the time saving is worth any offsets... It's hard to see the high level from down in SQL land
Keenan Brock
@kbrock
Jun 22 2016 15:33
yup
Joe Rafaniello
@jrafanie
Jun 22 2016 15:34
Plus, it's really fun to brag when you make a tiny change that is super awesome with no real downside ;-)
Keenan Brock
@kbrock
Jun 22 2016 15:34
same number of rows downloaded, 1 extra column
this is one of them :)
Joe Rafaniello
@jrafanie
Jun 22 2016 15:35
is this in cap and u processing?
elsewhere?
report generation?
Keenan Brock
@kbrock
Jun 22 2016 15:36
report generation (75% of the pages) and jason wanted for tree processing
we have virtual_attribute / column
but many of them were calculating count(*)
so this allows you to easily define that case
and it automatically makes it sortable (for a smal number of cases for now)
so when a user sorts by number of vms
Joe Rafaniello
@jrafanie
Jun 22 2016 15:37
Have you profiled/timed report generation that utilizes this feature? I'm curious what's the time memory saving/cost
Keenan Brock
@kbrock
Jun 22 2016 15:37
this avoids downloading every record, n+1 and just downloads 10 records
yes
Joe Rafaniello
@jrafanie
Jun 22 2016 15:37
Cool
Keenan Brock
@kbrock
Jun 22 2016 15:37
it is minutes vs 15 seconds
high level:
lets use sql limit / offset instead of downloading all and [5..10]
do you need numbers to prove that point?
I don't MiqPreloader.preload - that still downloads everything
the point is to just bring back the data you need
Joe Rafaniello
@jrafanie
Jun 22 2016 15:39
I think high level numbers help us understand where we'll see this benefit
it would help determine if things should be backported also
We use the reporting engine all over the UI so if we do this specific thing with reporting, I think reporting is the best thing to demonstrate even if you start with more isolated number comparisons
Oleg Barenboim
@chessbyte
Jun 22 2016 15:41
@jrafanie not clear if you are asking for more data from @kbrock
Keenan Brock
@kbrock
Jun 22 2016 15:41
I probably have done a bad job sharing
Do you understand the gist of why I want to add arel to virtual_columns?
And why I want to sort in the database instead of in memory?
from a high level?
Joe Rafaniello
@jrafanie
Jun 22 2016 15:43
I want high level why and where it helps, I can't follow the PR without digging into the code... don't have time to do that with every performance PR
Keenan Brock
@kbrock
Jun 22 2016 15:43
no
not this pr
in general
have I properly explained why I want to sort in the db instead of in ruby?
@jrafanie I can write it up if you need.
ok - I'll create an issue
this is ~ 40% of my work for tha past 1/2 year
it is all the same thing
it is scopes in rbac
virtual_arel, virtual_totals, my miq_expression changes
anyway - will do
Joe Rafaniello
@jrafanie
Jun 22 2016 15:46
I don't see why or where in this PR 9350
yes, that is where I'm failing. I'm assuming that doing the work in the db (and thus avoiding downloading everything and avoiding N+1) is self evident.
I will work harder to explain this in every PR
Joe Rafaniello
@jrafanie
Jun 22 2016 15:47
Yes, consolidate this into the description of the PR and make sure useful details are also in the commit messages and I'm good.
:+1:
Keenan Brock
@kbrock
Jun 22 2016 15:48
but if I go from N+1 to 1 query
you need me to prove it is better every time?
and run numbers every time?
I'm not talking about adding preload to download the whole database
I'm talking about modifying the query so we don't do the work at all
Joe Rafaniello
@jrafanie
Jun 22 2016 15:48
Only when you want to demonstrate numbers. Why + less queries without increased objects is enough for me.
Keenan Brock
@kbrock
Jun 22 2016 15:49
ok
but you are asking for numbers every time
Joe Rafaniello
@jrafanie
Jun 22 2016 15:50
I often get lost in the low levels details and can't remember where this code is even run
Keenan Brock
@kbrock
Jun 22 2016 15:50
yes, too many concepts
too much changing code
there is just too much logic in ruby
so we download too much data
Joe Rafaniello
@jrafanie
Jun 22 2016 15:51
I only need numbers when they better illustrate why
Keenan Brock
@kbrock
Jun 22 2016 15:51
my goal is to let the db do more
Oleg Barenboim
@chessbyte
Jun 22 2016 15:51
@kbrock agree with @jrafanie to provide reviewer a bit of context of where the performance fix is used and what the value is
Keenan Brock
@kbrock
Jun 22 2016 15:51
thanks
agreed
Joe Rafaniello
@jrafanie
Jun 22 2016 15:53
If you confirm you can remove a query with no downside in terms of objects and where this is beneficial, numbers are helpful when you either find an amazing breakthrough (10%+ with no downside) or if we have to decide on complexity vs. performance or something else
Nick LaMuro
@NickLaMuro
Jun 22 2016 15:54
This is where I tend to find the "links" section in the template helpful. Sounds like you would be repeating yourself a lot, so referencing past PRs to give context can also help avoid PR description bloat as well
Joe Rafaniello
@jrafanie
Jun 22 2016 15:54
In my opinion, minor changes on your path towards a goal only really need a WHY and WHERE
Yes, @NickLaMuro, good point... "As a followup from #1234, xxxx" can help
opening an issue with bullet points where you are going to do very similar general changes could also help so you don't have to explain why more than "less queries" in each PR
Keenan Brock
@kbrock
Jun 22 2016 18:40
Jason Frey
@Fryguy
Jun 22 2016 18:50
historically, calling attributes or [] on an AR model duped the entire internal attributes Hash
for getting a single value it's very expensive
Keenan Brock
@kbrock
Jun 22 2016 19:00
@Fryguy if you look at that response, I tried to follow the logic
@Fryguy but since it is taking up 97% less memory, even with the potential dup, can we let it slide?
This message was deleted
Keenan Brock
@kbrock
Jun 22 2016 19:11
@Fryguy Do you still like https://github.com/ManageIQ/manageiq/pull/8963/files ? It started as a pr to track down culprets. Not sure if keeping it protects us (in development) or if it has lived it's useful life and can be tossed