These are chat archives for ManageIQ/manageiq/performance

3rd
Feb 2016
Jason Frey
@Fryguy
Feb 03 2016 02:37 UTC
I would expect normalized_state to be called once per VM in the tree because that's what determines the icon
Keenan Brock
@kbrock
Feb 03 2016 02:37 UTC
yea
Jason Frey
@Fryguy
Feb 03 2016 02:38 UTC
it uses power_state and ext_management_system, so we preload for that in the TreeBuilder https://github.com/ManageIQ/manageiq/blob/master/app/presenters/tree_builder_vms_and_templates.rb#L25-L27
technically, normalized_state should belong in the decorators now that we have them, but that's a future item
we can probably drop that, if we changed those methods like archived? and orphaned? to just look at ems_id instead of the full association...same with the storage checks
it's not much gain, but it's a simple change
might save you that 3%
Keenan Brock
@kbrock
Feb 03 2016 02:41 UTC
yea - that could save us some
don't we need the ems for the icon?
you are saying the icon would be linked to the ems_id ?
Jason Frey
@Fryguy
Feb 03 2016 02:43 UTC
archived? and orphaned? just care about the presence of an ems...not the actual content
Keenan Brock
@kbrock
Feb 03 2016 02:43 UTC
aah - right
Jason Frey
@Fryguy
Feb 03 2016 02:43 UTC
the rest use power_state which is just a simple column
Keenan Brock
@kbrock
Feb 03 2016 02:43 UTC
yea - not sure if I put a pr in that changed that
aah
ringing a bell
... so many PRs ... so many ideas
Jason Frey
@Fryguy
Feb 03 2016 02:43 UTC
I can put in a quick PR, and you can check against your performance branches
give me a few mins...it's a good change anyway, I'd think
Keenan Brock
@kbrock
Feb 03 2016 02:44 UTC
it makes a difference
I have a scratch / PR with that too
you thinking 5.4?
Jason Frey
@Fryguy
Feb 03 2016 02:44 UTC
yeah
Keenan Brock
@kbrock
Feb 03 2016 02:44 UTC
ooh
yea - we have a bunch of joins for those
aah
in virtual colums
Jason Frey
@Fryguy
Feb 03 2016 02:45 UTC
it's probably not a ton of gain but I feel the low risk might be worth something
Keenan Brock
@kbrock
Feb 03 2016 02:45 UTC
I was doing virtual column changes
yea - a bunch of those really don't need to join
Jason Frey
@Fryguy
Feb 03 2016 02:46 UTC
ah yeah...but that's a bit more risky than the simple ext_management_system.nil? => ems_id.nil?
Keenan Brock
@kbrock
Feb 03 2016 02:46 UTC
ignoring the sql part
agreed
just saying - yes, checking nil id
I agree - great idea
Jason Frey
@Fryguy
Feb 03 2016 02:47 UTC
PR incoming
Keenan Brock
@kbrock
Feb 03 2016 02:47 UTC
I did both steps there, doing only 1 is MUCH less risky
the part I wondered
in archived / orphan
we do a to_a
at least on master
and we bring back all columns
that seemed a bit much
especially since we only display the name (and possibly look at the type / id / ems_id, storage_id)
Jason Frey
@Fryguy
Feb 03 2016 02:49 UTC
do you have a BZ number?
we don't do a to_a there...at least, I don't see it
Keenan Brock
@kbrock
Feb 03 2016 02:49 UTC
1281999
ok, must be in master
Jason Frey
@Fryguy
Feb 03 2016 02:51 UTC
ManageIQ/manageiq#6487
You can probably try this patch on 5.4 and see how it works for you
Keenan Brock
@kbrock
Feb 03 2016 02:52 UTC
k
ooh
while we're on this topic
Joe Rafaniello
@jrafanie
Feb 03 2016 02:53 UTC
yeah, that looks good
is it possible we're also preloading more than we need?
Keenan Brock
@kbrock
Feb 03 2016 02:53 UTC
that was changed
Jason Frey
@Fryguy
Feb 03 2016 02:53 UTC
it's possible, but not sure...technically, if we use the ems elsewhere, my removal of it from the TreeBuilderVmsAndTemplate could add an N+1, hence why we should definitely measure it before merging
Keenan Brock
@kbrock
Feb 03 2016 02:54 UTC
where is the code that loads all counts for a particular ems and caches them?
does this ring a bell?
Jason Frey
@Fryguy
Feb 03 2016 02:54 UTC
I am not set up to test the 10000 VM trees, so I'm hoping one of you guys can
yes, that sounds familiar
need to think about that
Keenan Brock
@kbrock
Feb 03 2016 02:55 UTC
aah - found it
storage.rb:419
Jason Frey
@Fryguy
Feb 03 2016 02:56 UTC
oh yeah...everywhere we use cache_with_timeout, it's a smell :D
The upside, is that it's easy to search for ;)
that's StorageFiles though, so totally unrelated to this performance effort
Keenan Brock
@kbrock
Feb 03 2016 02:57 UTC
can you compare unmanaged with managed_unregistered
what is the difference?
the code is the same
yes
well, they are counting on vms#ems_id IS NULL
that is why I thought of it
yea
well, both of those code blocks look identical to me
(note, there is a different block in the middle)
eh. I'll forget. and will remember in the future - ok. ignore
Jason Frey
@Fryguy
Feb 03 2016 03:02 UTC
oooh a test failed :)
Joe Rafaniello
@jrafanie
Feb 03 2016 03:02 UTC
@kbrock are you see 20% of time spent in arrange_nodes? https://github.com/stefankroes/ancestry/blob/v1.2.5/lib/ancestry/class_methods.rb#L44
Keenan Brock
@kbrock
Feb 03 2016 03:03 UTC
@jrafanie a bunch - yes.
heck - THAT is why I reached out to ancestry gem people
wanted to change THEIR gem
for that one method
Joe Rafaniello
@jrafanie
Feb 03 2016 03:03 UTC
yeah
Keenan Brock
@kbrock
Feb 03 2016 03:04 UTC
the issue is they support so many version of rails
as does active hash
tricky to keep up with the times and support rails 3
good news though
I got case and sort w/ null ordering into arel
Jason Frey
@Fryguy
Feb 03 2016 03:05 UTC
still doesn't seem like something that should be taking that long
Keenan Brock
@kbrock
Feb 03 2016 03:05 UTC
specifically for the sort ordering in ancestry
but not sure about forcing a newer version of arel
ugh
EMS_REF yet anoth define const in an rspec strikes again
Jason Frey
@Fryguy
Feb 03 2016 03:06 UTC
ugh
Keenan Brock
@kbrock
Feb 03 2016 03:07 UTC
need 5 seconds - easy enough to fix
Jason Frey
@Fryguy
Feb 03 2016 03:07 UTC
link?
Keenan Brock
@kbrock
Feb 03 2016 03:07 UTC
I'm seeing the deprecation warnings in your run
Jason Frey
@Fryguy
Feb 03 2016 03:07 UTC
why is that defined as a constant (-‸ლ)
Keenan Brock
@kbrock
Feb 03 2016 03:08 UTC
yea
easy to fix
Jason Frey
@Fryguy
Feb 03 2016 03:08 UTC
I just don't understand why that was even merged :(
Keenan Brock
@kbrock
Feb 03 2016 03:08 UTC
heh
is that a let or just hardcode the string twice?
ooh
Jason Frey
@Fryguy
Feb 03 2016 03:08 UTC
a let
or even an instance variable
Keenan Brock
@kbrock
Feb 03 2016 03:08 UTC
a few of them - yea
Jason Frey
@Fryguy
Feb 03 2016 03:09 UTC
oh, please kill them all...they are just problems waiting to happen
Keenan Brock
@kbrock
Feb 03 2016 03:09 UTC
lol
people make fun of me here
I hate these deprecation warnings
but yea
problems just waiting to happen
@fryguy is there a rubocop for constants in specs?
Jason Frey
@Fryguy
Feb 03 2016 03:11 UTC
there is a rubocop-rspec gem, I think, but I remember not enabling it for some reason
I don't know if it has a check for that though
Keenan Brock
@kbrock
Feb 03 2016 03:12 UTC
yea - think it gets too many warnings
Joe Rafaniello
@jrafanie
Feb 03 2016 03:12 UTC

still doesn't seem like something that should be taking that long

ancestor_ids is called 22969 times, nearly 3% of the total time https://github.com/stefankroes/ancestry/blob/v1.2.5/lib/ancestry/instance_methods.rb#L69

Jason Frey
@Fryguy
Feb 03 2016 03:12 UTC
so, 2-3 times per VM?
Joe Rafaniello
@jrafanie
Feb 03 2016 03:12 UTC
yes, I guess it depends on the relationships
Jason Frey
@Fryguy
Feb 03 2016 03:13 UTC
yeah I can see how splitting that string and casting over and over could be expensive
it is splitting the ancestry column, which is a string like "123/456/789"
@jrafanie is ancestor_ids getting called 23k times completely inside arrange_nodes?
Joe Rafaniello
@jrafanie
Feb 03 2016 03:16 UTC
yeah, but the whole method arrange_nodes method is just called so many times, the id columns are cast to integer 1.3 million times
yes
Jason Frey
@Fryguy
Feb 03 2016 03:16 UTC
O_O
why the heck is it called so many times
Jason Frey
@Fryguy
Feb 03 2016 03:18 UTC
I thought there were 10000 VMs...plus a few folders
that's only ~10000 nodes
unless I'm missing something
Joe Rafaniello
@jrafanie
Feb 03 2016 03:20 UTC
I can look at it more tomorrow... this red section is the area I want to look at more tomorrow... I think that's the area we can possibly make a dent... minus the stuff already found
Jason Frey
@Fryguy
Feb 03 2016 03:20 UTC
:+1: yeah it's late
Joe Rafaniello
@jrafanie
Feb 03 2016 03:20 UTC
Screen Shot 2016-02-02 at 10.19.04 PM.png
Jason Frey
@Fryguy
Feb 03 2016 03:21 UTC
I have to go to a dentist appointment tomorrow, so I'm making up for it now ;)
Joe Rafaniello
@jrafanie
Feb 03 2016 03:21 UTC
nested injects are killing my soul
Jason Frey
@Fryguy
Feb 03 2016 03:21 UTC
well, that makes sense...there are 6 EMSes :)
convert inject to each_with_object in your mind and all is well
Joe Rafaniello
@jrafanie
Feb 03 2016 03:22 UTC
yeah
the other areas appear to be 1-2% optimizations
oh well, good night
Keenan Brock
@kbrock
Feb 03 2016 03:26 UTC
@jrafanie yea - that is a classic cartissian product
just like the sort we fixed today
@jrafanie night
Joe Rafaniello
@jrafanie
Feb 03 2016 03:27 UTC
@kbrock just holler if you need help getting rubyprof working with rack request profiling
Jason Frey
@Fryguy
Feb 03 2016 03:27 UTC
gnite :moon:
Joe Rafaniello
@jrafanie
Feb 03 2016 03:27 UTC
it's pretty sweet
Greg Blomquist
@blomquisg
Feb 03 2016 03:27 UTC

gotobedalready

Keenan Brock
@kbrock
Feb 03 2016 03:28 UTC
@Fryguy are you going to change that spec
I assume it had an id that was invalid
our code has :dependent => null, so that shouldn't ever happen
@blomquisg <3
Oleg Barenboim
@chessbyte
Feb 03 2016 03:28 UTC
@blomquisg they don't stop until the high-severity performance bug is squashed ;-)
Greg Blomquist
@blomquisg
Feb 03 2016 03:28 UTC
Haha, hashtag turns it into title b/c markdown
Keenan Brock
@kbrock
Feb 03 2016 03:28 UTC
heh
@jrafanie thanks
@jrafanie yea - I've been manually adding that stuff to rack-mini-profiler
will have to run this app in ruby 2.2 and try that
@Fryguy what is the latest vesion of ruby you think I can get 5.4 running (to use better ruby profiling tools)
Oleg Barenboim
@chessbyte
Feb 03 2016 03:32 UTC
you guys rock - getting to the bottom of this issue
Jason Frey
@Fryguy
Feb 03 2016 03:32 UTC
@kbrock I'm not sure what version would work
@kbrock your "simple" PR has a lot more changes than I expected ;)
Keenan Brock
@kbrock
Feb 03 2016 03:33 UTC
1 line?
hmm
the deprecation warning?
Jason Frey
@Fryguy
Feb 03 2016 03:34 UTC
the problem is the constant was defined inside the before(:each), so it's technically "redefined" on every pass
Keenan Brock
@kbrock
Feb 03 2016 03:34 UTC
we have like 25 specs with constants defined in them
Jason Frey
@Fryguy
Feb 03 2016 03:34 UTC
all the other constants are defined globally, and are not really as much of a problem as the EMS_REF one
Keenan Brock
@kbrock
Feb 03 2016 03:34 UTC
but constants are defined in the global space
yes
ok
Jason Frey
@Fryguy
Feb 03 2016 03:34 UTC
yes, so you'll only have a problem on name collision
Keenan Brock
@kbrock
Feb 03 2016 03:34 UTC
remove the others?
they are wrong
Jason Frey
@Fryguy
Feb 03 2016 03:34 UTC
it shadows the real problem...but I agree they should be lets instead
Keenan Brock
@kbrock
Feb 03 2016 03:34 UTC
but no complaints / warnings
Jason Frey
@Fryguy
Feb 03 2016 03:35 UTC
yeah...I just went to the PR expecting a 1 line change and got 50...haha
Keenan Brock
@kbrock
Feb 03 2016 03:35 UTC
lol
well
I thought, I'd just hit the 3 files that have the problems
and then after like 5 I said "eject"
and saw there were many more
and cried a little inside
Jason Frey
@Fryguy
Feb 03 2016 03:35 UTC
so this isn't all of them?
Keenan Brock
@kbrock
Feb 03 2016 03:35 UTC
no
MAYBE half
Jason Frey
@Fryguy
Feb 03 2016 03:35 UTC
ok
Keenan Brock
@kbrock
Feb 03 2016 03:35 UTC
probably less
Jason Frey
@Fryguy
Feb 03 2016 03:36 UTC
you got some syntax errors
and some issues (I commented where I saw)
Keenan Brock
@kbrock
Feb 03 2016 03:36 UTC
ag -G '_spec.rb' '[A-Z][A-Z].*[^=]=[^>]' spec/
ugggh
thanks
Jason Frey
@Fryguy
Feb 03 2016 03:37 UTC
@blomquisg Are we pinging your phone? :telephone: :imp:
Oleg Barenboim
@chessbyte
Feb 03 2016 03:43 UTC
@Fryguy so is #6487 supposed to make an impact to performance?
Keenan Brock
@kbrock
Feb 03 2016 03:44 UTC
@chessbyte yes, it removes repopulating of resources and checks columns directly in the query
Jason Frey
@Fryguy
Feb 03 2016 03:45 UTC
@chessbyte a little...I didn't measure
Oleg Barenboim
@chessbyte
Feb 03 2016 03:45 UTC
@Fryguy @kbrock I wonder if it is cheaper to convert them to actual columns and teach EmsRefresh to set them
Jason Frey
@Fryguy
Feb 03 2016 03:45 UTC
but the low risk change made sense, even if it only gives a small percent
you mean archived, orphaned, etc?
Oleg Barenboim
@chessbyte
Feb 03 2016 03:45 UTC
yes
Jason Frey
@Fryguy
Feb 03 2016 03:46 UTC
possibly...even so, this change is equivalent to that, performance wise, now
but a real column would make reporting better
search, etc
Oleg Barenboim
@chessbyte
Feb 03 2016 03:46 UTC
they can only be changed during EmsRefresh (I assume) and there is no sense in recomputing them on each method invocation
just my 2 cents ;-)
Keenan Brock
@kbrock
Feb 03 2016 03:47 UTC
well, after matthewd's rails 5 goes in, I want to get my PR in that will make it possible to sort by virtual columns
it is working, but conflicts with his changes - so holding off
Jason Frey
@Fryguy
Feb 03 2016 04:01 UTC
@chessbyte I agree it's a good idea...we probably should review other similar calculated columns as well
Keenan Brock
@kbrock
Feb 03 2016 04:02 UTC
@Fryguy there are a bunch of them
Joe Rafaniello
@jrafanie
Feb 03 2016 14:47 UTC

@Fryguy what is the latest vesion of ruby you think I can get 5.4 running (to use better ruby profiling tools)

@kbrock I have 2.1 running for stackprof support

I don't know that 2.2 would add any help for profiling
Joe Rafaniello
@jrafanie
Feb 03 2016 15:08 UTC
Can I merge ManageIQ/manageiq#6487
looks good
@kbrock is your sort fix in rbac committed anywhere?
Keenan Brock
@kbrock
Feb 03 2016 15:10 UTC
For things like ManageIQ/manageiq#6487 do we need numbers before and after?
@jrafanie thanks. I'll get it into master too
hard to live in 5.4 and master at the same time
Joe Rafaniello
@jrafanie
Feb 03 2016 15:11 UTC
yes, I agree, I was going to truncate ems_events and backup my db before trying to migrate to master
Keenan Brock
@kbrock
Feb 03 2016 15:12 UTC
@jrafanie locally, I've pulled out rbac and others into multiple functions
so I can get an idea of where the time is spent
does it make sense to get this into master?
or the difference with older versions makes it harder to back port?
just 1 gigantic method is hard for me to narrow down timing
especially when the timings I get tell you rollup per method
Joe Rafaniello
@jrafanie
Feb 03 2016 15:13 UTC
it depends, I personally don't see much we can do in rbac land that would make that much impact
Keenan Brock
@kbrock
Feb 03 2016 15:13 UTC
lol
impact performance wise, or understanding wise?
Joe Rafaniello
@jrafanie
Feb 03 2016 15:14 UTC
if we could avoid using rbac search twice to fetch the same vms twice, that would be a gain
Keenan Brock
@kbrock
Feb 03 2016 15:14 UTC
when I read the code
it is hard to know where the time is spent
(and hard to know what a gigantic method is doing)
Joe Rafaniello
@jrafanie
Feb 03 2016 15:14 UTC
rubyprof's callstack graph makes it much easier IMHO
Keenan Brock
@kbrock
Feb 03 2016 15:15 UTC
so you don't introduce methods to organize the code you are reading?
Joe Rafaniello
@jrafanie
Feb 03 2016 15:17 UTC
not really, nothing really jumps out at me here (5.4.z without the sort in rbac search fix)
Screen Shot 2016-02-03 at 10.17.05 AM.png
(second visit to vm_infra/explorer)
Jason Frey
@Fryguy
Feb 03 2016 15:21 UTC
what is that casting code in the sort there?
are we using a sort_by but indexing inside it?...it look s so weird
Matthew Draper
@matthewd
Feb 03 2016 15:21 UTC
That whole sort node is the one we killed, I think
Joe Rafaniello
@jrafanie
Feb 03 2016 15:21 UTC
yes
this is pre sort fix
Jason Frey
@Fryguy
Feb 03 2016 15:21 UTC
ah...so this is before that fix
Joe Rafaniello
@jrafanie
Feb 03 2016 15:21 UTC
so it's the .id cals
targets = targets.sort { |a,b| target_ids.index(a.id) <=> target_ids.index(b.id) }
Jason Frey
@Fryguy
Feb 03 2016 15:22 UTC
yeah, ok good
Joe Rafaniello
@jrafanie
Feb 03 2016 15:22 UTC
we see it again in the ancestry call
Jason Frey
@Fryguy
Feb 03 2016 15:22 UTC
I thought you found another one ;)
Matthew Draper
@matthewd
Feb 03 2016 15:22 UTC
So, this entire prune_rbac seems fairly exhausted to me
Joe Rafaniello
@jrafanie
Feb 03 2016 15:23 UTC
if you mean, there's not much else we can do, then yes, I agree
Matthew Draper
@matthewd
Feb 03 2016 15:23 UTC
Barring sufficient cleverness to avoid whatever queries it's doing entirely, doesn't seem likely to squeeze much else out
Joe Rafaniello
@jrafanie
Feb 03 2016 15:23 UTC
maybe if we find out why setting default values takes so much time, we might get a few %
Keenan Brock
@kbrock
Feb 03 2016 15:24 UTC
ManageIQ/manageiq#6495
... default values in the db ...
Joe Rafaniello
@jrafanie
Feb 03 2016 15:24 UTC
but part of the problem is we instantiate the same vms in the relationship walking as we do in the prune_rbac
Keenan Brock
@kbrock
Feb 03 2016 15:24 UTC
we are missing a bunch of inverse_of
Jason Frey
@Fryguy
Feb 03 2016 15:24 UTC
do default values in the DB work when you do Model.new?
Keenan Brock
@kbrock
Feb 03 2016 15:24 UTC
@Fryguy yes
@matthewd verify?
Matthew Draper
@matthewd
Feb 03 2016 15:26 UTC
@jrafanie is there anything else interesting outside prune_rbac?
Joe Rafaniello
@jrafanie
Feb 03 2016 15:26 UTC
FYI, DefaultValueFor::InstanceMethod#set_default_values takes up 1.86% and 1.88% for the 10k+ vms that get instantiated twice
Matthew Draper
@matthewd
Feb 03 2016 15:26 UTC
Can you push that HTML somewhere?
Joe Rafaniello
@jrafanie
Feb 03 2016 15:26 UTC
yeah, can I put it here?
Matthew Draper
@matthewd
Feb 03 2016 15:26 UTC
gist, maybe?
Keenan Brock
@kbrock
Feb 03 2016 15:27 UTC
think there is a way to post a gist and be able to view it in html
know markdown works
Matthew Draper
@matthewd
Feb 03 2016 15:27 UTC
@kbrock missing inverses sounds plausible, though I dunno how much it'll save us
Joe Rafaniello
@jrafanie
Feb 03 2016 15:28 UTC
on 3.2?
is that a problem there too?
Keenan Brock
@kbrock
Feb 03 2016 15:28 UTC
@matthewd ManageIQ/manageiq#5982
@matthewd do have a script that shows all of them
will run in a little bit
Joe Rafaniello
@jrafanie
Feb 03 2016 15:29 UTC
Jason Frey
@Fryguy
Feb 03 2016 15:30 UTC
I used to have a Chrome plugin...let me see if it still works
Joe Rafaniello
@jrafanie
Feb 03 2016 15:30 UTC
i can put them somewhere else if needed
Matthew Draper
@matthewd
Feb 03 2016 15:30 UTC
Yep, that works thanks
Joe Rafaniello
@jrafanie
Feb 03 2016 15:31 UTC
let me gist the second click
oh wait
that was the second click, let me rename
Screen Shot 2016-02-02 at 10.19.04 PM.png
I want to look at the ancestry code, above
we also call to_json 3 times taking up 15% of the time, not sure if a newer json is faster
Matthew Draper
@matthewd
Feb 03 2016 15:41 UTC
check_for_circular_references sounds surprisingly expensive, at a glance
I guess it's just the sum of a lot of (individually quite fast) calls
Daniel Berger
@djberg96
Feb 03 2016 15:51 UTC
welp, time to change all the .each calls to for loops
Keenan Brock
@kbrock
Feb 03 2016 15:58 UTC
@jrafanie if you rename file to index.html then this will work: http://bl.ocks.org/jrafanie/0e0b30468e00c762325b
Jason Frey
@Fryguy
Feb 03 2016 16:06 UTC
that site is pretty cool
Keenan Brock
@kbrock
Feb 03 2016 16:06 UTC
from the d3js people
cool
Jason Frey
@Fryguy
Feb 03 2016 16:08 UTC
how do you get the link? is it just the gist id?
ah i got...there's a chrome extension too :)
I need to change the HTML to fit the svg to the view
Keenan Brock
@kbrock
Feb 03 2016 16:13 UTC
there is a raw option
I have a local chrome extension that does it
didn't know blocks had one already - very cool
Keenan Brock
@kbrock
Feb 03 2016 18:00 UTC
is there an easy way so VmOrTemplate(id: integer, ...r: string)#allocated_disk_storage displays as VmOrTemplate#allocated_disk_storage ?
that is like 5 lines
do I hack ActiveRecord::Base.inspect?
Matthew Draper
@matthewd
Feb 03 2016 18:01 UTC
Yeah
Keenan Brock
@kbrock
Feb 03 2016 18:01 UTC
hmm. 5.4 uses miq version of rails
maybe an initializer can monkey patch
thanks
Keenan Brock
@kbrock
Feb 03 2016 18:25 UTC
siri please autocorrect: Vm.method(:inspect).location (source, to_source?)
aah - source_location
Joe Rafaniello
@jrafanie
Feb 03 2016 18:44 UTC
Are we even using CimProfiles?
Keenan Brock
@kbrock
Feb 03 2016 18:44 UTC
I find myself asking that question about a bunch of stuff :)
Joe Rafaniello
@jrafanie
Feb 03 2016 18:45 UTC
It's so frustrating looking at perf stuff and random other classes are being loaded
Joe Rafaniello
@jrafanie
Feb 03 2016 18:54 UTC
@kbrock @dmetzger57 did we discuss MiqExpression.model_details?
Dennis Metzger
@dmetzger57
Feb 03 2016 18:55 UTC
@jrafanie not that i recall
Joe Rafaniello
@jrafanie
Feb 03 2016 18:56 UTC
7% of the first visit to vm_infra/explorer on ruby 2.1 on 5.4.z (without the sort fix) is in building cached data for 2 models
we used to pre-populate this information at startup in all environments... I don't see why we couldn't do it a UI worker context at startup of that process
Dennis Metzger
@dmetzger57
Feb 03 2016 19:00 UTC
that would at least partially explain the extra slow render on the first visit to the page, but it won’t help with the every other visit
Joe Rafaniello
@jrafanie
Feb 03 2016 19:00 UTC
Screen Shot 2016-02-03 at 1.59.42 PM.png
Yes, that would only help the first visit
Dennis Metzger
@dmetzger57
Feb 03 2016 19:03 UTC
not sure if its worth the work/risk for the current BZ, definitely another going forward perf improvement
Joe Rafaniello
@jrafanie
Feb 03 2016 19:07 UTC
yeah, it would depend if the initial visit is what they're most disliking
Dennis Metzger
@dmetzger57
Feb 03 2016 19:08 UTC
it’s the 30-40 seconds everytime they visit the page that is the current complaint
Joe Rafaniello
@jrafanie
Feb 03 2016 19:08 UTC
ok, the change is minor, just wanted to point it out
diff --git a/vmdb/lib/workers/mixins/web_server_worker_mixin.rb b/vmdb/lib/workers/mixins/web_server_worker_mixin.rb
index 1a2874d..eea775c 100644
--- a/vmdb/lib/workers/mixins/web_server_worker_mixin.rb
+++ b/vmdb/lib/workers/mixins/web_server_worker_mixin.rb
@@ -11,6 +11,11 @@ module WebServerWorkerMixin
   def do_before_work_loop
     @worker.release_db_connection

+    MiqExpression.base_tables.each do |model|
+      MiqExpression.get_relats(model)
+      MiqExpression.build_lists(model)
+    end
+
     # Since thin traps interrupts, log that we're going away and update our worker row
     at_exit { do_exit("Exit request received.") }
   end
or something like that
Oleg Barenboim
@chessbyte
Feb 03 2016 19:14 UTC
Why do we have to build anything at all? Why can't it be built on first reference via standard memoization pattern??
Less is More
Chris Arcand
@chrisarcand
Feb 03 2016 19:15 UTC
Joe Rafaniello
@jrafanie
Feb 03 2016 19:15 UTC
It's built on demand right now, I don't really know what it does
ok, @Fryguy tell me again why we need to build all 3 trees when we only have 1 visible?
Oleg Barenboim
@chessbyte
Feb 03 2016 19:18 UTC
I am so lost on all this pre-building for the UI so we dont spend extra time if and only if user clicks somewhere
suggest getting back to basics
Joe Rafaniello
@jrafanie
Feb 03 2016 19:23 UTC
we call VmShowMixin#build_vm_tree 3 times, once per tree... each build_vm_tree uses relationships to build a relationship tree, fetching 10k+ things from the database... we then filter that tree by doing a different search using Rbac.search (10k mostly duplicate things)
the trees are all closed initially and only 1 is visible
once these enormous trees, with data we don't actually see at first, are built, we convert all three 3 to json so we can have a format the UI can then draw I assume
if we're lucky, we might be able to carve off 10-20% by finding slow code
Oleg Barenboim
@chessbyte
Feb 03 2016 19:30 UTC
@jrafanie this is where javascripting that page over REST API to get what is needed when it is needed would help, right?
Joe Rafaniello
@jrafanie
Feb 03 2016 19:32 UTC
well, yes, I think the "dividing into parts that we need when we need them" is the key, the vehicle of delivering these parts is immaterial
If a single vm changes power state, we'd have to reload the 3 whole trees to see it
if REST is the vehicle to force the carving up of things into parts, then yes, REST all the things
Matthew Draper
@matthewd
Feb 03 2016 19:39 UTC
Doesn't that put us right back where we were before @Fryguy fixed it? :confused:
Joe Rafaniello
@jrafanie
Feb 03 2016 19:56 UTC
@dmetzger57 what's the expectations?
I might have missed it, have we done comparisons of master to 5.4?
I haven't yet migrated to master... I'm wondering if it's worth doing that
Joe Rafaniello
@jrafanie
Feb 03 2016 20:02 UTC
@kbrock looks like a valid test failure on your rbac.search sort fix: https://travis-ci.org/ManageIQ/manageiq/jobs/106761189
Dennis Metzger
@dmetzger57
Feb 03 2016 20:08 UTC
@jrafanie @akross has run 5.4.4.2, 5.4.5.0 and 5.5.0.13 with the customer database. results in BZ 1281999. The number are better on 5.5.0, on the Haswell hardware the page loads in 12-17 seconds as opposed to 27-20 wih 5.4.42 and on Sandy Bridge its 21-27 seconds vs 49-63 seconds. Master to be tried. I’ve asked if the times on 5.5.0 would be acceptable, though I’ve been told that an upgrade is not viable in the short term.
Keenan Brock
@kbrock
Feb 03 2016 20:12 UTC
@jrafanie thanks for the reminder
Joe Rafaniello
@jrafanie
Feb 03 2016 20:12 UTC
np
Joe Rafaniello
@jrafanie
Feb 03 2016 20:23 UTC
@dmetzger57 @akrzos in the comments regarding the timing in the bugzilla, https://bugzilla.redhat.com/show_bug.cgi?id=1281999#c21, is that the initial visit to vm_infra/explorer and is that from the rails log/production.log?
Keenan Brock
@kbrock
Feb 03 2016 20:24 UTC
the initial visit has a bunch of junk in there - including that one part that you mentioned lj
Joe Rafaniello
@jrafanie
Feb 03 2016 20:24 UTC
Yeah, I can't tell what the numbers mean
Keenan Brock
@kbrock
Feb 03 2016 20:24 UTC
I'd prefer to ignore the first, and just look at the others
yea
Dan Clarizio
@dclarizio
Feb 03 2016 20:32 UTC
@chessbyte We could not build the second and third accordion trees until clicked on, but then those should NOT be the ones taking all the time as those are just trees of filters (maybe dozens) not thousands like the VM & Templates tree, which mimics the VMware hierarchy. That first tree is the biggest offender in large environments.
Alex Krzos
@akrzos
Feb 03 2016 20:32 UTC
@jrafanie Yes thats from the production.log
The initial visit to the explorer is the first timing
(always higher)
Dan Clarizio
@dclarizio
Feb 03 2016 20:33 UTC
@chessbyte We used to add nodes to the trees as the user clicked thru, but then as @Fryguy pointed out, after quite a few nodes were open, leaving and revisiting the tree would cause n+1 type fetches to fill in the parts the user had opened prior
so, @Fryguy optimized that to build the entire tree at the start . . . which has the consequences of taking a long time in super large environments . . . so kind of a catch 22
Keenan Brock
@kbrock
Feb 03 2016 20:34 UTC
but those trees are only 3 levels deep?
Dan Clarizio
@dclarizio
Feb 03 2016 20:35 UTC
@kbrock, the second and third accordion trees, yes
those should also build quickly as we don't populate the VMs in the actual trees
Oleg Barenboim
@chessbyte
Feb 03 2016 20:35 UTC
@dclarizio thanks for the explanation
Keenan Brock
@kbrock
Feb 03 2016 20:36 UTC
thanks
Dan Clarizio
@dclarizio
Feb 03 2016 20:37 UTC
so moving to REST will just move the problem . . . what I'd REALLY like to do is not put the VMs in the tree, just show them on the right, but that would take buy in from PM as it would no longer look like the provider we originally tried to mimic
Keenan Brock
@kbrock
Feb 03 2016 21:03 UTC

found this:

records = sliced.map { |slice| records_for(slice) }.flatten
# vs flat_map

but it is in rails - no need to patch old rails at this time

Keenan Brock
@kbrock
Feb 03 2016 21:26 UTC
wow - in VMs, default_value_for shows up all over the place
can we just put those types of values into a before_validation XX, :on => :create?
Jason Frey
@Fryguy
Feb 03 2016 21:33 UTC
:point_up: February 3, 2016 2:23 PM we preload the whole tree for performance...try clicking and opening like folders and the refresh the page, and you'll be happy with the preloading :)
that's what I was trying to explain yesterday :/
Keenan Brock
@kbrock
Feb 03 2016 21:34 UTC
12% of our render time is forming json
Jason Frey
@Fryguy
Feb 03 2016 21:34 UTC
the other option is to go back to the old way of tree walking, but somehow do it efficiently
I couldn't figure out how to make that spaghetti efficient :(
so 88% is not forming JSON
Keenan Brock
@kbrock
Feb 03 2016 21:35 UTC
well, can we make a change like that to 5.4?
Jason Frey
@Fryguy
Feb 03 2016 21:35 UTC
probably not :D
Keenan Brock
@kbrock
Feb 03 2016 21:35 UTC
well, 79% is from building the left hand tree that becomes the json?
@matthewd I have a config/initializer with this:
module ActiveRecord
  module Core
    module ClassMethods
      def inspect
        self.name # super
      end
    end
  end
end
Jason Frey
@Fryguy
Feb 03 2016 21:37 UTC
:point_up: February 3, 2016 4:26 PM The whole pint of default_value_for is that it fills in the values properly on .new, .create, and .find ... All of the other methods don't do that
Keenan Brock
@kbrock
Feb 03 2016 21:37 UTC
it isn't fixing the names. other ideas?
we need for find?
Jason Frey
@Fryguy
Feb 03 2016 21:38 UTC
well, it doesn't do anything on find, and that is the point
Keenan Brock
@kbrock
Feb 03 2016 21:38 UTC
it does
Jason Frey
@Fryguy
Feb 03 2016 21:38 UTC
so nil and false can be stored in the database and it doesn't set a new default
Keenan Brock
@kbrock
Feb 03 2016 21:39 UTC
but when you instantiate those objects, we take a performance hit
Jason Frey
@Fryguy
Feb 03 2016 21:39 UTC
Well, it does patch initialize, but it should be a noop on find
Keenan Brock
@kbrock
Feb 03 2016 21:39 UTC
I thought it did set a new default if the value was nil
Jason Frey
@Fryguy
Feb 03 2016 21:39 UTC
no, it doesn't
only on new or create
Keenan Brock
@kbrock
Feb 03 2016 21:41 UTC
aah
allows_nil (default: true)
  Sets explicitly passed nil values if option is set to true
ok, the docs are funky ref - if set to false, then it will change the value on find too
Jason Frey
@Fryguy
Feb 03 2016 21:44 UTC
we don't ever change the defaults
Keenan Brock
@kbrock
Feb 03 2016 21:44 UTC
yea - I remembered that the plugin will do that. just wanted to make sure it wasn't on by default
Matthew Draper
@matthewd
Feb 03 2016 21:49 UTC
@kbrock does that fix User.inspect ?
Keenan Brock
@kbrock
Feb 03 2016 21:49 UTC
no
@matthewd get full "pretty" inspects
tried super and self.name
Matthew Draper
@matthewd
Feb 03 2016 21:50 UTC
If you literally call the inspect method by name, it doesn't do what you've defined it to do?
Keenan Brock
@kbrock
Feb 03 2016 21:50 UTC
if a module gets mixed in, and I change the module. does that change for those objects where it is mixed in already?
@matthewd correct
 Vm.inspect
=> "Vm(id: integer, vendor: string, format: string, version: string, name: string, description: text, location: string, config_xml: string, autostart: string, host_id: integer, last_sync_on: datetime, created_on: datetime, updated_on: datetime, storage_id: integer, guid: string, ems_id: integer, last_scan_on: datetime, last_scan_attempt_on: datetime, uid_ems: string, retires_on: date, retired: boolean, boot_time: datetime, tools_status: string, standby_action: string, power_state: string, state_changed_on: datetime, previous_state: string, connection_state: string, last_perf_capture_on: datetime, registered: boolean, busy: boolean, smart: boolean, memory_reserve: integer, memory_reserve_expand: boolean, memory_limit: integer, memory_shares: integer, memory_shares_level: string, cpu_reserve: integer, cpu_reserve_expand: boolean, cpu_limit: integer, cpu_shares: integer, cpu_shares_level: string, cpu_affinity: string, ems_created_on: datetime, template: boolean, evm_owner_id: integer, ems_ref_obj: string, miq_group_id: integer, linked_clone: boolean, fault_tolerance: boolean, type: string, ems_ref: string, ems_cluster_id: integer, retirement_warn: integer, retirement_last_warn: datetime, vnc_port: integer, flavor_id: integer, availability_zone_id: integer, cloud: boolean, retirement_state: string, cloud_network_id: integer, cloud_subnet_id: integer, cloud_tenant_id: integer, raw_power_state: string, publicly_available: boolean, orchestration_stack_id: integer, retirement_requester: string)"
yea
I typed the above into the console
and still doesn't work
class VM ; def inspect ; name ; end ;end works
now Vm.inspect ==> short
Matthew Draper
@matthewd
Feb 03 2016 21:52 UTC
[1] pry(main)> User.inspect
=> "User(id: integer, name: string, email: string, icon: string, created_on: datetime, updated_on: datetime, userid: string, settings: text, filters: text, lastlogon: datetime, lastlogoff: datetime, region: integer, current_group_id: integer, first_name: string, last_name: string, password_digest: string)"
[2] pry(main)> module ActiveRecord::Core::ClassMethods; def inspect; name; end; end
=> :inspect
[3] pry(main)> User.inspect
=> "User"
Jason Frey
@Fryguy
Feb 03 2016 21:52 UTC
@kbrock Why don't you just call .name or .to_s directly? Or is some tool calling inspect?
Keenan Brock
@kbrock
Feb 03 2016 21:52 UTC
 module ActiveRecord::Core::ClassMethods; def inspect; name; end; end
=> :inspect
irb(main):016:0> Vm.inspect
=> "Vm(id: integer, vendor: .....
Vm.method(:inspect).source_location
=> ["/Users/kbrock/.gem/ruby/2.1.5/bundler/gems/rails-4842a8377644/activerecord/lib/active_record/base.rb", 418]
cool - thanks.
think I found it
tool calling inspect
Matthew Draper
@matthewd
Feb 03 2016 21:54 UTC
module ActiveRecord::Core::ClassMethods; def inspect; name; end; end
NameError: uninitialized constant ActiveRecord::Core
Keenan Brock
@kbrock
Feb 03 2016 21:54 UTC
ruby-prof
Jason Frey
@Fryguy
Feb 03 2016 21:55 UTC
ah yeah, I remember ruby-prof doing that...I had had a helper class for ruby-prof to deal with that in the past, but I can't find it anymore
Keenan Brock
@kbrock
Feb 03 2016 21:55 UTC
thanks matthew
you solved it
module ActiveRecord ; class Base ; def self.inspect ; name ; end ; end ; end
Matthew Draper
@matthewd
Feb 03 2016 21:56 UTC
@kbrock btw I made a thing for you: rails/rails#23457
Jason Frey
@Fryguy
Feb 03 2016 21:58 UTC
@matthewd What does that change buy?
Matthew Draper
@matthewd
Feb 03 2016 21:58 UTC
It gives us a place we can ~safely patch, to teach order(:virtual_column) how to find a suitable Arel expression
(and make where(:virtual_column => 'foo') work, as a bonus — in theory, at least)
Jason Frey
@Fryguy
Feb 03 2016 22:00 UTC
oh nice
I can talk Rails 5 PR now if you want...just need to move to another room
unless it's too late, then we can move to tommorrow
it's up to you @matthewd
Matthew Draper
@matthewd
Feb 03 2016 22:02 UTC
Sure!
Oleg Barenboim
@chessbyte
Feb 03 2016 22:02 UTC
@Fryguy please don't merge Rails5 PR without discussing with me
Jason Frey
@Fryguy
Feb 03 2016 22:02 UTC
nope...we are just reviewing
@matthewd wanted to do a walkthrough review
Joe Rafaniello
@jrafanie
Feb 03 2016 22:03 UTC
tomorrow is today for @matthewd ;-)
Oleg Barenboim
@chessbyte
Feb 03 2016 22:03 UTC
I discussed this PR conceptually with @tenderlove and think the 4 of us need to get on the same page about risk/reward of WHEN we merge Rails 5 PR
Matthew Draper
@matthewd
Feb 03 2016 22:04 UTC
==> @tenderlove considers it risky to merge at the moment?
Oleg Barenboim
@chessbyte
Feb 03 2016 22:05 UTC
definitely not before Rails 5 goes GA - and after it goes GA, the 4 of us need to agree on the when
Jason Frey
@Fryguy
Feb 03 2016 22:05 UTC
yes, I agree
Oleg Barenboim
@chessbyte
Feb 03 2016 22:05 UTC
vis-a-vis Darga and downstream release
Matthew Draper
@matthewd
Feb 03 2016 22:06 UTC
Well there's little point reviewing it now if we're not merging for 6 months
Oleg Barenboim
@chessbyte
Feb 03 2016 22:06 UTC
6 months?
Jason Frey
@Fryguy
Feb 03 2016 22:06 UTC
@chessbyte what is your time frame for delay...roughly?
I was thinking weeks not months...but maybe I'm off
Oleg Barenboim
@chessbyte
Feb 03 2016 22:07 UTC
something we need to discuss -- it may be days or weeks or maybe a few months -- depending on what comes out of the discussion
Jason Frey
@Fryguy
Feb 03 2016 22:07 UTC
The release targets from here are RC1 on February 16 and then final on February 23
Matthew Draper
@matthewd
Feb 03 2016 22:08 UTC
If we're not merging soon enough for @kbrock (in particular) to use the new stuff, I don't see why we'd then risk dropping it in late in the cycle
Jason Frey
@Fryguy
Feb 03 2016 22:09 UTC
@chessbyte Can you set up a meeting, and then we can discuss, and then after that I'll review with @matthewd ?
Oleg Barenboim
@chessbyte
Feb 03 2016 22:09 UTC
@matthewd that is why I suggest we discuss this as well as the Darga release schedule in one BlueJeans meeting
ok - I think we are in agreement on the need for a meeting
Jason Frey
@Fryguy
Feb 03 2016 22:09 UTC
I agree with @matthewd that there's no point in review if the merge isn't relatively soon
Oleg Barenboim
@chessbyte
Feb 03 2016 22:10 UTC
@matthewd don't be so negative - I think we need to do what is best for the ManageIQ community -- maybe our discussion will pan out that will be merging Rails 5 sooner rather than later - or maybe it will not
does 3pm NYC time tomorrow work for you @matthewd?
Matthew Draper
@matthewd
Feb 03 2016 22:11 UTC
I'm not being negative, just averse to risk without benefit
Yup
Jason Frey
@Fryguy
Feb 03 2016 22:13 UTC
works for me
Oleg Barenboim
@chessbyte
Feb 03 2016 22:13 UTC
ok - set it up for tomorrow @ 3
Matthew Draper
@matthewd
Feb 03 2016 22:14 UTC
:+1:
Keenan Brock
@kbrock
Feb 03 2016 22:16 UTC
@matthewd yea, I like the point from sgrif - pulling out the table meta would allow joining to the same model a number of times (but different relations)
Matthew Draper
@matthewd
Feb 03 2016 22:16 UTC
@kbrock that's a different thing
Keenan Brock
@kbrock
Feb 03 2016 22:17 UTC
@matthewd what is the chance of us changing: RBac.filtered(nil) === Rbac.filtered(all) and Rbac.search([]) ==> [] # (no op) ?
er, what is needed?
RBac.filtered([]) == [] # no change and Rbac.search(:targets => nil) == all # no change
Matthew Draper
@matthewd
Feb 03 2016 22:18 UTC
@kbrock last time we discussed it, I think you were going to add warnings to the form we wanted to change
Keenan Brock
@kbrock
Feb 03 2016 22:18 UTC
thanks
that is one of 3 linch-pins
Keenan Brock
@kbrock
Feb 03 2016 22:26 UTC
@Fryguy that def archived ; ems_id.nil? ; end removed 1.6% from the show page
remembered liking it before, but that one is trivial / great
@Fryguy do we backport these
how much do we want to do with 5.4 / 5.5?
Jason Frey
@Fryguy
Feb 03 2016 22:28 UTC
yeha, they can easily be backported
Keenan Brock
@kbrock
Feb 03 2016 22:28 UTC
well - it CAN be backported
how much do we do?
Jason Frey
@Fryguy
Feb 03 2016 22:29 UTC
depends on the customer needs
I guess it's up to @dmetzger57 and @jonnyfiveiq
Dennis Metzger
@dmetzger57
Feb 03 2016 22:37 UTC
@kbrock @Fryguy from what I’ve gathered, page rending in the 15 second range (currently in the 40 second range) is what the customer is looking for. I’ve not recieved a definitive “under XX seconds” requirement yet.
Keenan Brock
@kbrock
Feb 03 2016 22:38 UTC
thanks
Joe Rafaniello
@jrafanie
Feb 03 2016 23:21 UTC
with @kbrock's sort change and @Fryguy's archived/orphaned changed locally for me 39, 31 improved to 26, 21
Keenan Brock
@kbrock
Feb 03 2016 23:25 UTC
:)
Jason Frey
@Fryguy
Feb 03 2016 23:32 UTC
so ~30-33%
not bad for two relatively simple changes :)
Keenan Brock
@kbrock
Feb 03 2016 23:40 UTC
AND we got those changes into master too
everyone wins