These are chat archives for ManageIQ/manageiq/performance

23rd
Aug 2016
Keenan Brock
@kbrock
Aug 23 2016 00:38
@NickLaMuro nice pr - is that darga yes?
Nick LaMuro
@NickLaMuro
Aug 23 2016 00:41
should be, don't see why not
Keenan Brock
@kbrock
Aug 23 2016 00:44
It looks good to merge
any last thoughts?
Nick LaMuro
@NickLaMuro
Aug 23 2016 00:46
Uh, I was going to try to address your comments, unless you want to do that later
cuz now that I have had dinner myself, the _miq_group_primary_key method is just stupid...
and I want to erase that from any git history :)
clearly I was drunk
and this is your periodic reminder that @NickLaMuro doesn't drink, and the above comment was a joke
I will try to make the changes quickly here so you can get in your PR for the day :)
Nick LaMuro
@NickLaMuro
Aug 23 2016 00:55
Yeah, just got rid of about 10 lines of stupidity
Keenan Brock
@kbrock
Aug 23 2016 00:55
the code was fine
Nick LaMuro
@NickLaMuro
Aug 23 2016 00:55
I was able to get rid of the class methods entirely and was less "cheeky" in the process
Keenan Brock
@kbrock
Aug 23 2016 00:56
heh
lol
I had a beer tonight
trying to drink more
support the wife
Nick LaMuro
@NickLaMuro
Aug 23 2016 00:57
hah
Keenan Brock
@kbrock
Aug 23 2016 00:57
well, honestly
maybey why this is funny
Nick LaMuro
@NickLaMuro
Aug 23 2016 01:02
This will probably send @kbrock on a squirrel chasing expedition, but I am kinda curious where 30k object came from between this PR and the last...
but it might be unrelated to what I changed
I didn't redo the base case metrics... because I didn't want to wait around for 7 min...
Keenan Brock
@kbrock
Aug 23 2016 01:03
yup
the inner join
vs sub select
big difference
also note the # objects is not accurate
well
some grain of salt
it introduces objects
Nick LaMuro
@NickLaMuro
Aug 23 2016 01:05
Was comparing ManageIQ/manageiq#10685 to ManageIQ/manageiq#10290
which I was getting them in the same fashion
so the "incorrectness" is shared
@kbrock but yeah, changes pushed
Keenan Brock
@kbrock
Aug 23 2016 01:25
is it miq group id
or miq_owner_group_id?
Nick LaMuro
@NickLaMuro
Aug 23 2016 03:23
should be miq_group_id because it is just a belongs_to
Keenan Brock
@kbrock
Aug 23 2016 03:53
yes, I was thinking about the owner bug
good catch
in the future, think I'd prefer examples to match the data, rather than matching the sql
Keenan Brock
@kbrock
Aug 23 2016 14:20
@NickLaMuro ok, so ladas found a good bug
I made scopes flow a little more through rbac
and the references block is screwing it up
throw a references in there
you force a "join"
and when you want to bring back records, that forces a distinct
and if you have an order clause with a distinct (or any aggregate), then that forces the order value to be in the select clause
and if you use order by lower(column) - you then need to add lower(column) to the query
Oleg Barenboim
@chessbyte
Aug 23 2016 14:23
@NickLaMuro I am blown away by the performance improvement in ManageIQ/manageiq#10685 :heart: :clap: THANK YOU!
from 93seconds to sub-second is amazing -- 100x improvement - WOW
actually closer to 200x
Chris Arcand
@chrisarcand
Aug 23 2016 14:27
:+1: :+1: :clap:
Nick LaMuro
@NickLaMuro
Aug 23 2016 14:41
@kbrock was that an existing bug (before that bug fix PR I did for Dan), or after?
Keenan Brock
@kbrock
Aug 23 2016 14:41
no
not you
same block of code though
so I changed some code to have a scope flow through (instead of a separate query that just hardcoded ids)
two steps forward...
BUT
hardcoded ids are good because it is easier to deal with
throw in a scope
and now you have rails changing it
Nick LaMuro
@NickLaMuro
Aug 23 2016 14:44
whew...
cool cool
Keenan Brock
@kbrock
Aug 23 2016 14:44
but
if we can fix this
we get a BIG win
right across the board
the question - do we even need these references?
Nick LaMuro
@NickLaMuro
Aug 23 2016 14:45
hrmmm, tough to answer
Keenan Brock
@kbrock
Aug 23 2016 14:46
well, if we have a where hash
Nick LaMuro
@NickLaMuro
Aug 23 2016 14:46
basically, do we ever do a WHERE against the joined tables?
Keenan Brock
@kbrock
Aug 23 2016 14:46
we would know right?
good news
BEFORE - we didn't have good whereselect clauses
or even selectwhere clauses
now, we have proper select clauses
so we know from selects, if we need the column for the references there
string where, not sure what we can do
hash where, we should have it
(and hopefully, we'll be able to get rid of those inner queries - I am pushing them yes, but we both want to get rid of them asap)
@NickLaMuro is this in english?
Nick LaMuro
@NickLaMuro
Aug 23 2016 14:49
Kinda, but I also don't have the context you have from looking into this
but I think I get the gist
basically the where/select can determine if we even need to toss in the references at all
Keenan Brock
@kbrock
Aug 23 2016 14:50
pipedream: just provide the where, select, and order clauses
from there, derive the includes / references
Nick LaMuro
@NickLaMuro
Aug 23 2016 14:51
I think that would assume that your caller isn't going to want to make use of the Rails active relation stuff to pull in associated records
Keenan Brock
@kbrock
Aug 23 2016 14:51
how so?
up until recently, rbac made the actual query
it is only recently that I got scopes flowing through
Chris Arcand
@chrisarcand
Aug 23 2016 14:52
IMO the more general filtering/scoping taken out of RBAC the better.
Keenan Brock
@kbrock
Aug 23 2016 14:52
@chrisarcand yes
Chris Arcand
@chrisarcand
Aug 23 2016 14:52
RBAC should be nothing but your role/permissions filtering but is…basically everything right now. :(
Keenan Brock
@kbrock
Aug 23 2016 14:52
plan is to first get scopes coming into rbac, and let rbac tack some stuff on
then move that tack on stuff into caller
Chris Arcand
@chrisarcand
Aug 23 2016 14:53

then move that tack on stuff into caller

?

Keenan Brock
@kbrock
Aug 23 2016 14:53
+1
ooh
# before
Rbac.filter(Vm.all, :conditions => {:id => 5})
# after
Rbac.filter(Vm.where(:id => 5))
in the 2nd one, the :conditions was moved into the caller of rbac
in the 1st one, rbac is responsible for tacking some stuff on - think we agree that is bad
Chris Arcand
@chrisarcand
Aug 23 2016 14:56
Right; I was confused. “first get scopes coming in to rbac” == “then move that tack of stuff into the caller"
:+1:
Keenan Brock
@kbrock
Aug 23 2016 14:56
yes, nick and I were up too late last night
Chris Arcand
@chrisarcand
Aug 23 2016 14:56
Imagine that ;)
Keenan Brock
@kbrock
Aug 23 2016 14:56
;)
this is just an example
there is a crap ton of include_for_finds in the repo
so we would need to do a lot of cleanup if we were to remove includes from Rbac
not saying we shouldn't, just saying there is work to do if that is the goal
... which probably also went without saying
Chris Arcand
@chrisarcand
Aug 23 2016 15:03
O_o Not even sure what that is
Keenan Brock
@kbrock
Aug 23 2016 15:05
yes, I want to remove (ignore) that exact line
that was require before because they would just mention a column before, not making sure it was valid
@NickLaMuro @dmetzger57 sorry - relocating, need a few minutes
Oleg Barenboim
@chessbyte
Aug 23 2016 15:15
On a certain PR, I see Performance/Casecmp - Use casecmp instead of downcase ==. Is that a real performance concern or nothing to worry about?
this is pure Ruby; no SQL
Jason Frey
@Fryguy
Aug 23 2016 15:15
very minor performance benefit, but I think the bot will detect that
Oleg Barenboim
@chessbyte
Aug 23 2016 15:16
yes, it did -- wondering if I should push to get that addressed
pretty minor to fix
Jason Frey
@Fryguy
Aug 23 2016 15:16
I don't see why it shouldn't be fixed
unless the PR author wasn't the one that introduced it
The downside to casecmp is the signature ... it returns 0 or -1
makes the conditional very C-like
Keenan Brock
@kbrock
Aug 23 2016 15:46
I like reading a == b
or a.downcase == b.downcase
sure seems that ruby could optimize that for us under the coverse... but anyay
Keenan Brock
@kbrock
Aug 23 2016 18:48

@Fryguy @jrafanie I'm starting to want the specs and functionality from ManageIQ/manageiq#10476
Could you take a brief look at the proposed solution? specificially:

virtual_delegate :num_cpu, :to => "hardware.cpu_sockets", :allow_nil => true, :default => 0

there is a lot of future code contingent upon that and I'd like to merge sooner rather than later

we use a lot of non standard delegates in our code.
Keenan Brock
@kbrock
Aug 23 2016 22:00

Pulled those specs out and fixed some tests in ManageIQ/manageiq#10717

@Fryguy if you could look at the new virtual_delegate syntax. So I can change if you don't like the new :to syntax. (It is necessary because we don't follow rails conventions in all our virtual column names)

happy hacking all
and wow @NickLaMuro - very happy how self contained these new fixes are.
Keenan Brock
@kbrock
Aug 23 2016 22:06
Tangent: https://github.com/rails/rails/compare/master...kbrock:arel_aliases
With that change to rails, we can Vm.select(:virtual_attribute)... one can dream - right?