These are chat archives for ManageIQ/manageiq/performance

12th
Sep 2016
Nick LaMuro
@NickLaMuro
Sep 12 2016 14:08
I did, but I need to rework it to match my patch from before
Keenan Brock
@kbrock
Sep 12 2016 14:09
NICK!
well, that patch is rockin
want to get it in
just updated 11171 - the updated test shows the difference.
Plan on moving that code over to others
Nick LaMuro
@NickLaMuro
Sep 12 2016 14:20
I am going to be figuring out what is being cached by that class variable though doing some "puts" debugging
As noted in ManageIQ/manageiq#10929 (where I lost my "Dev Card"), there should be a file digest for the most part
but the images might be from the DB on this tree, but I am not sure how that all works under the hood in MIQ
I just noticed in the stackproof when running in production mode that it was hitting the same method that I was seeing earlier (incorrectly though, because I was in development mode)
Keenan Brock
@kbrock
Sep 12 2016 15:12
just noticed that ManageIQ/manageiq#8870 has not been back ported
So if a customer uses MiqExpression, it will still use ruby instead of sql
I felt this was too risky at the time.
But it was applied in May to master and we would have found a bug if there is one
@dmetzger57 ^ thoughts?
big impact, and the patch has been vetted for 4 months on master
Dennis Metzger
@dmetzger57
Sep 12 2016 15:28
:+1:
Keenan Brock
@kbrock
Sep 12 2016 15:30
so I create a BZ - "don't be slow" and assign the bz to pr?
Dennis Metzger
@dmetzger57
Sep 12 2016 15:37
I think darga/yes should be sufficient in this case
Nick LaMuro
@NickLaMuro
Sep 12 2016 16:30
@kbrock Here is the PR with the asset patch: ManageIQ/manageiq#10929
Updated the code and description
Nick LaMuro
@NickLaMuro
Sep 12 2016 16:39
err... s/description/comments
Keenan Brock
@kbrock
Sep 12 2016 16:51
is there a reason this is only for node_icon?
our code all over has issues with image_path being too slow
Nick LaMuro
@NickLaMuro
Sep 12 2016 16:52

From https://github.com/ManageIQ/manageiq/pull/10929#issuecomment-246403927 :smile:

Not sure if there is a better place to add this that is more inclusive to this, so I am open to suggestions on a better place for this cache live, but it cuts the render time (after Keenan's patches) for /service/explorer in half on large trees.

So yes, I agree, this is a focused solution for something that probably could be solved in a broader fashion. Didn't know where a good place for that was after a first pass though
Keenan Brock
@kbrock
Sep 12 2016 17:00
yea, I guess that is my way of saying "this is so great we should apply this to everywhere in the code"
cache all things
Nick LaMuro
@NickLaMuro
Sep 12 2016 17:00
yeah, wasn't sure of idle memory implications at scale though
Keenan Brock
@kbrock
Sep 12 2016 17:01
+1
would be nice if rails just did the right thing
they cache all of this stuff :(
Nick LaMuro
@NickLaMuro
Sep 12 2016 17:01
well, in this case, this is our stuff
the images are ones from the DB, and we are using sprockets to resolve the path for us
but they wouldn't be included in the file digest because they aren't "assets"
"images from the DB", aka "customer/custom images"
Oleg Barenboim
@chessbyte
Sep 12 2016 17:21
@kbrock for ManageIQ/manageiq#8221, getting strange message when backporting
$ git cherry-pick -e -x -m 1 5b5fee4   
On branch darga
Your branch is up-to-date with 'upstream/darga'.
You are currently cherry-picking commit 5b5fee4.
Keenan Brock
@kbrock
Sep 12 2016 17:22
huh
maybe it is in there
Oleg Barenboim
@chessbyte
Sep 12 2016 17:23
@kbrock can you try cherry-picking locally
Keenan Brock
@kbrock
Sep 12 2016 17:23
ok
Oleg Barenboim
@chessbyte
Sep 12 2016 17:23
I am in a meeting - and cannot dive into details
@kbrock can you make a Darga PR for ManageIQ/manageiq#10717
Keenan Brock
@kbrock
Sep 12 2016 17:31
@chessbyte It got merged. you did it right. I misread git. (commit was not showing, merge commit was.)
Oleg Barenboim
@chessbyte
Sep 12 2016 17:31
thx
Nick LaMuro
@NickLaMuro
Sep 12 2016 19:27
@kbrock Between ManageIQ/manageiq#11166 and ManageIQ/manageiq#11171 , which one are you going with?
Keenan Brock
@kbrock
Sep 12 2016 19:40
I'll close 111666 - that one is kludge (but doesn't change the infrastructure)
I prefer 71 much more - from there we can use ancestry everywhere
ugh
Nick LaMuro
@NickLaMuro
Sep 12 2016 19:41
I see now that 11171 is more invasive than 11166
Keenan Brock
@kbrock
Sep 12 2016 19:41
but it is cleaner
actually
do you have suggestions how to make the find children / other not modify that core function so much
Nick LaMuro
@NickLaMuro
Sep 12 2016 19:42
still groking at this point
Keenan Brock
@kbrock
Sep 12 2016 19:42
it was the if check in the middle object[:...] that was the problem
it is a little tricky since the each calling the code converts a hash to an array pair, but I tried to isolate that
Nick LaMuro
@NickLaMuro
Sep 12 2016 19:44
FYI, by "Invasive", I don't mean that it == bad/worse solution, but == Higher risk
Keenan Brock
@kbrock
Sep 12 2016 19:46
+1
ok
our current bug
miq_expression has been taught that you can use arel in a virtual attribute
so if you have miq expression say vm.disconnected => false, but disconnected is a virtual attribute, it actually constructs the subquery foo and puts it in there
Nick LaMuro
@NickLaMuro
Sep 12 2016 19:47
puts on context switching hat
Keenan Brock
@kbrock
Sep 12 2016 19:47
ok
this is a fryingpan => volcano
Nick LaMuro
@NickLaMuro
Sep 12 2016 19:48
okay, I follow the above so far
Dennis Metzger
@dmetzger57
Sep 12 2016 19:48
this is a fryingpan => volcano == Build Blocker :smile:
Keenan Brock
@kbrock
Sep 12 2016 19:49
virtual attribute + arel = goodnes (same page)
miq expression used to not understand that
we educated it
in master, it is well educated. it constructs it properly with a sub query
in darga, it just throws it into sql

master:

bundle exec rspec spec/lib/rbac/filterer_spec.rb:988
line filterer.rb:201 ( exp_sql = ...)

"miq_groups"."id" IN (
  SELECT DISTINCT "vms"."miq_group_id"
  FROM "vms" WHERE (
    "vms"."connection_state" IS NULL OR "vms"."connection_state" != 'connected'
  ) = 'f'
)
Nick LaMuro
@NickLaMuro
Sep 12 2016 19:54
All makes sense. I assume this is one of those "we didn't backport for risk reasons but backported something else that required those features" issues
Keenan Brock
@kbrock
Sep 12 2016 19:54
darga:
bundle exec rspec rspec ./spec/models/rbac_spec.rb:1088
rbac.rb:444 (exp_sql = ...)
miq_groups.id IN (
  SELECT DISTINCT miq_group_id
  FROM vms WHERE disconnected = 'false'
)
+1
I'm hitting a lot of those today :(
and wishing I had been more aggressive about backporting more
good news, I'm kinda the only person who has been messing with miq_expression
tim too
but I don't see his stuff in here
ugh, but I'm not seeing it
Keenan Brock
@kbrock
Sep 12 2016 20:03
cool. the file was moved, --follow shows it right away
@NickLaMuro ok, just to have fun. today I said "you know what, I want the miq expression stuff backported" ==> ManageIQ/manageiq#10717
which is part of the solution here
the actual bug at play is the test introduced is testing this new behavior that was not merged
I was expecting this code to just say "I can't do it" very surprised it tried to generate that sql at all
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:16
That test doesn't seem to be testing the new behavior, but testing that the results returned are correct. It should fall back to ruby when the column doesn't exist, correct?
Keenan Brock
@kbrock
Sep 12 2016 20:16
there are ~4 tests around that
what does "doesn't exist" mean?
if it is a virtual column with arel...
we want that to generate sql
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:17
sorry, s/column/column or virtual_column
Keenan Brock
@kbrock
Sep 12 2016 20:18
so yes, you are correct
and that column is a virtual column
so it should generate sql
Keenan Brock
@kbrock
Sep 12 2016 20:19
yes
so on darga, it is doing that correctly
er
it is detecting "we should do this in sql"
I think
but when it tries to generate
it isn't coming up with the arel
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:20
the alternative being, it is incorrectly determining that it can generate via SQL, but doing it anyway
Keenan Brock
@kbrock
Sep 12 2016 20:20
and you know what
we may have not altered any behavior
but changed the test and it exposes a bug (that we fixed)
because to be honest
this should NOT have generated sql
it should have said not supported
because the code that supports this hasn't been merged yet
(I realized that this morning and was trying to get it merged)
but alas, when I apply those changes, it still does not work
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:25
lol, did man git and am still surprised to see this:
NAME
          git - the stupid content tracker
Keenan Brock
@kbrock
Sep 12 2016 20:25
ooh
ManageIQ/manageiq#8870
it was backported
ok, so now I have to figure out what we did on master after that commit to stabalize it
I mean, I know that isn't "really" fixing the bug, but is there a reason that got changed?
Keenan Brock
@kbrock
Sep 12 2016 20:34
yea
the old version was optimizing to using the database
so I had to pick one that didn't get fixed
it said "pick a bad column"
but as we improve our code, there are fewer and fewer "bad columns"
so, I'm tempted to roll this back
but then I remember
we are going to be on the hotspot to improve performance soon, and then I'll really need this patch in there
just like delegation
we can't say "don't use miq_expressions" - that will be a 2 minute hit
this patch is mandatory, otherwise most of our virtual_attribute fixes will fall down if they are used in miq_expressions
Oleg Barenboim
@chessbyte
Sep 12 2016 20:37
@dmetzger57 @kbrock this seems to be dragging on, we need to get darga green -- please make a revert PR
Keenan Brock
@kbrock
Sep 12 2016 20:37
I will do that
this is setting us up come october
give me 3
@chessbyte ManageIQ/manageiq#11205
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:46
@chessbyte Seeing as ManageIQ/manageiq#10685 kinda banks on #8870 being in place, is that BZ still worth being attached to ManageIQ/manageiq#10685 ?
Oleg Barenboim
@chessbyte
Sep 12 2016 20:46
you mentioned the same BZ twice
Keenan Brock
@kbrock
Sep 12 2016 20:47
nick - we can use virtual sql. we just get hit if the user uses miq_expression with that attribute
so we get the gain if it is in the primary query or sorted by it
just get a big hit from miq expressions
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:48
That page uses MiqExpression to generate that view
Keenan Brock
@kbrock
Sep 12 2016 20:48
ooh
lol
Oleg Barenboim
@chessbyte
Sep 12 2016 20:48
guys we are putting out a new release later this year, so not sure you should focus THAT much on getting every performance fix backported
Keenan Brock
@kbrock
Sep 12 2016 20:49
I only care about darga in october
Oleg Barenboim
@chessbyte
Sep 12 2016 20:49
what about euwe in november/december
Keenan Brock
@kbrock
Sep 12 2016 20:49
euwe will rock
we have a lot of room left, but it will rock
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:50
@chessbyte Not actually concerned about getting the code in, just about adding code that is effectively useless
Also, I added a bunch of tests, so my guess is they are going to brake now
Dennis Metzger
@dmetzger57
Sep 12 2016 20:53
@NickLaMuro so there is harm in leaving it in, correct. in that case reverting it would seem appropriate
Nick LaMuro
@NickLaMuro
Sep 12 2016 20:54
@kbrock 's PR will be the litmus test with that. If tests fail, my guess is that would be the cause
or at least one of them
But yes, I agree that your statement of "reverting it would seem appropriate"
Keenan Brock
@kbrock
Sep 12 2016 20:55
agreed
it is the right course of action
just saying, we need to find a way to get it to our customers
so we need to find a way to get it to pass
again, I think my problem is I've been too hesitant for the past month to mark code as darga/yes, so we've been missing necessary PRs
Oleg Barenboim
@chessbyte
Sep 12 2016 20:57
at this point, I would prefer Darga PRs that you and Nick can validate locally
Keenan Brock
@kbrock
Sep 12 2016 20:57
yes
Oleg Barenboim
@chessbyte
Sep 12 2016 20:57
for CFME 5.6.2, that is
Keenan Brock
@kbrock
Sep 12 2016 20:57
it has been on master for 3-4 months
sounds great
will do
Oleg Barenboim
@chessbyte
Sep 12 2016 20:57
thx
Keenan Brock
@kbrock
Sep 12 2016 20:57
and insead of cherry-pick, we'll create PRs
Oleg Barenboim
@chessbyte
Sep 12 2016 20:58
but I want Darga green ASAP so we are not blocking Satoe
make sense?
see email I just forwarded re build
Nick LaMuro
@NickLaMuro
Sep 12 2016 21:06
:point_up: September 12, 2016 3:46 PM I take back this statement partially, I was under the impression that #8870 has been in darga for a while now... my bad
s/partially/mostly
I will verify on my own that it still provides a performance benefit, just to make sure, but shouldn't stop the build work