These are chat archives for ManageIQ/manageiq/performance

7th
Sep 2016
Keenan Brock
@kbrock
Sep 07 2016 13:34
@NickLaMuro I think it would do us some good revisiting relationships in the tree. could get across the board wins there
Keenan Brock
@kbrock
Sep 07 2016 14:05
@Fryguy are there special meta calls into relationship_mixin? There look to be a bunch of methods that are not called
e.g.: subtree_ids_arranged, depth
Keenan Brock
@kbrock
Sep 07 2016 14:21
@NickLaMuro FWIW/ when I boot I always set:
      ENV["ERB_IN_CONFIG"]="true"
      ENV["SKIP_PRIMORDIAL_SEED"]="true"
      ENV["RACK_MINI_PROFILER_PATCH"]="notifications"
google used to have a profiler protocol to show performance on the server in the ui
but it was put aside - suposed to come back.
they read a header from the request and make a separate http request to the server to fetch the profiling data.
I asked illiya gigorik (sp) about it. he said coming back.
I really like the idea of not having to be on server to figure out which page I just hit and the numbers
also, it is difficult to tap into # queries for count / and pluck so if you want to understand that, we will have to monkeypatch rails
Jason Frey
@Fryguy
Sep 07 2016 15:41
@kbrock I wrote relationship_mixin as a full API which mirrors the ancestry gem
Keenan Brock
@kbrock
Sep 07 2016 15:42
@Fryguy ok, so you've rather me fix everything in a pattern. vs fix ones used.
Jason Frey
@Fryguy
Sep 07 2016 15:42
So, whether or not it's used is not important
think of it like a gem (in fact, I've been thinking of gemifying it as something like ancestry-polymorphic or something)
yeah, fix all in a pattern
it should be fully covered, otherwise we should add specs
Keenan Brock
@kbrock
Sep 07 2016 15:43
there are #{x}_ids but it brings back [[class, id],[class id]]
Jason Frey
@Fryguy
Sep 07 2016 15:43
yeah, because polymorphic
it could bring back different types
Keenan Brock
@kbrock
Sep 07 2016 15:44
but we always seem to just call resource_pairs_to_ids on it
and we only ever call it with :of_type => ...
so if we change, we'd leave in scopes vs bringing back all relations then converting to all actual objects
then to filter
then to pull out the id / class
then just the id
in one place- we then call size
Jason Frey
@Fryguy
Sep 07 2016 15:46
so for relationship mixin I've made a clear separation in my mind between how it's used and the API
thinking like a gem
Keenan Brock
@kbrock
Sep 07 2016 15:46
ok
but
this is a hot spot for us
Jason Frey
@Fryguy
Sep 07 2016 15:46
now, the code could optimize internally if it sees a pattern
if it sees of_type with a single entity, then internally go a different route
Keenan Brock
@kbrock
Sep 07 2016 15:46
I keep coming back to the code not because I don't like it
because it keeps showing up as a problem
Jason Frey
@Fryguy
Sep 07 2016 15:46
define hot spot
Keenan Brock
@kbrock
Sep 07 2016 15:47
bottle neck
Jason Frey
@Fryguy
Sep 07 2016 15:47
LOL
Keenan Brock
@kbrock
Sep 07 2016 15:47
a spot that uses up a lot of time / queries / memory
fixing this code is hard jason
Jason Frey
@Fryguy
Sep 07 2016 15:47
are we talking 20% of every call type bottleneck? 80%?
Keenan Brock
@kbrock
Sep 07 2016 15:48
remember last time we looked at blue trees
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:48
one sec... I got some data
Keenan Brock
@kbrock
Sep 07 2016 15:48
and it made the process 20% faster?
just changing 1 stupid method?
it is hard refactoring this stuff. that pr hasn't been merged yet
Oleg Barenboim
@chessbyte
Sep 07 2016 15:49
@kbrock - data please
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:49
Brace your selfs... big old copy-pasta incoming...
Jason Frey
@Fryguy
Sep 07 2016 15:49
UH OH :)
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:49
Stackprof from /vm_infra/explorer
==================================
  Mode: wall(1000)
  Samples: 7572 (0.70% miss rate)
  GC: 825 (10.90%)
==================================
     TOTAL    (pct)     SAMPLES    (pct)     FRAME
      3106  (41.0%)        3106  (41.0%)     Puma::Single#run
      1394  (18.4%)         997  (13.2%)     ActiveRecord::LazyAttributeHash#[]
       361   (4.8%)         361   (4.8%)     ActiveRecord::Attribute#initialize
       405   (5.3%)         258   (3.4%)     ActiveRecord::Attribute#value
       154   (2.0%)         154   (2.0%)     Relationship#ancestry_base_class
       249   (3.3%)         133   (1.8%)     Ancestry::InstanceMethods#primary_key_type
       106   (1.4%)         106   (1.4%)     block in ActiveRecord::Result#hash_rows
        68   (0.9%)          68   (0.9%)     Relationship.ancestry_column
        78   (1.0%)          52   (0.7%)     block (2 levels) in Class#class_attribute
       299   (3.9%)          50   (0.7%)     Ancestry::InstanceMethods#cast_primary_key
      2276  (30.1%)          47   (0.6%)     block in Ancestry::ClassMethods#arrange_nodes
       108   (1.4%)          47   (0.6%)     ActiveModel::Type::Value#cast
        46   (0.6%)          46   (0.6%)     ActiveRecord::Core#init_internals
        46   (0.6%)          46   (0.6%)     ActiveRecord::LazyAttributeHash#key?
        49   (0.6%)          44   (0.6%)     ActiveRecord::Core::ClassMethods#allocate
      1325  (17.5%)          41   (0.5%)     block (3 levels) in Ancestry::ClassMethods#arrange_nodes
        39   (0.5%)          39   (0.5%)     ActiveRecord::ModelSchema::ClassMethods#schema_loaded?
      1884  (24.9%)          38   (0.5%)     ActiveRecord::AttributeMethods::Read#_read_attribute
        36   (0.5%)          36   (0.5%)     ActiveModel::Type::ImmutableString#cast_value
        34   (0.4%)          34   (0.4%)     block (4 levels) in Class#class_attribute
       397   (5.2%)          32   (0.4%)     ActiveRecord::LazyAttributeHash#assign_default_value
        96   (1.3%)          32   (0.4%)     ActiveRecord::ModelSchema::ClassMethods#inheritance_column
        30   (0.4%)          30   (0.4%)     #<Module:0x007fda9acd3238>.convert
        43   (0.6%)          27   (0.4%)     block in Delegator.delegating_block
        26   (0.3%)          26   (0.3%)     ActiveRecord::LazyAttributeHash#initialize
      1420  (18.8%)          26   (0.3%)     ActiveRecord::AttributeSet#[]
        61   (0.8%)          25   (0.3%)     ActiveModel::Type::String#cast_value
        24   (0.3%)          24   (0.3%)     ActiveRecord::Core::ClassMethods#===
        90   (1.2%)          23   (0.3%)     ActiveSupport::ToJsonWithActiveSupportEncoder#to_json
        23   (0.3%)          23   (0.3%)     ActiveModel::Type::Integer#deserialize
Jason Frey
@Fryguy
Sep 07 2016 15:49
I get what I ask for haha
Keenan Brock
@kbrock
Sep 07 2016 15:49
ManageIQ/manageiq#10160
Jason Frey
@Fryguy
Sep 07 2016 15:50
oh ok, so this is specifically about folder_path_objs
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:50
Basically alot of the CPU time is spent in the Ancestry gem handling determining the polymorphic relationship on each of the nodes in the tree
Jason Frey
@Fryguy
Sep 07 2016 15:51
(which we have to get back to)
Keenan Brock
@kbrock
Sep 07 2016 15:51
that was 1 example
I just found another today
Jason Frey
@Fryguy
Sep 07 2016 15:51
gotcha
Keenan Brock
@kbrock
Sep 07 2016 15:51
but I keep finding patterns. and I wanted to just delete the code rather than improve code where I don't have numbers (because it isn't called)
Oleg Barenboim
@chessbyte
Sep 07 2016 15:51
@kbrock @NickLaMuro which PRs need attention from @Fryguy ?
Keenan Brock
@kbrock
Sep 07 2016 15:51
but I understand the pattern thought. so moving forward
Oleg Barenboim
@chessbyte
Sep 07 2016 15:51
or do you want to have a design discussion?
Jason Frey
@Fryguy
Sep 07 2016 15:51
-1 on deleting it just because it isn't used...it's a proper API that mirrors ancestry
Keenan Brock
@kbrock
Sep 07 2016 15:52
@Fryguy gotcha
@chessbyte that was my question
just wanted to know if I can delete slow stuff
Jason Frey
@Fryguy
Sep 07 2016 15:52
but, to your point, optimizing code we don't use doesn't make sense to put effort into
Keenan Brock
@kbrock
Sep 07 2016 15:52
the discussion digressed because I got "prove it was slow"
Jason Frey
@Fryguy
Sep 07 2016 15:52
unless it's like a 5 minute job because of a pattern
Keenan Brock
@kbrock
Sep 07 2016 15:52
+1
Jason Frey
@Fryguy
Sep 07 2016 15:52
I wasn't asking prove it was slow...I know it's slow :sweat_smile:
Keenan Brock
@kbrock
Sep 07 2016 15:52
lol
Jason Frey
@Fryguy
Sep 07 2016 15:52
I was asking where you are experiencing the slowness and how frequently you see the problem
because you said "hot spot" I wasn't sure if that translated to "every single tree is slow by X%"
or just in a couple specific cases
Oleg Barenboim
@chessbyte
Sep 07 2016 15:54
@kbrock @NickLaMuro you guys live in performance-land. When asking for advice/help, please give big picture
Jason Frey
@Fryguy
Sep 07 2016 15:54
@NickLaMuro in your data dump there...I only see Relationship at 2%?
Oleg Barenboim
@chessbyte
Sep 07 2016 15:54
PRs, BZs, general summaries of outside-in issues you are solving -- would be helpful to @Fryguy and me
Jason Frey
@Fryguy
Sep 07 2016 15:55
(hard to tell)
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:55
@Fryguy Sorry, it isn't completely clear with that, and it is better with the dump so you can go up the stack with it
but the percentages there are basically spots where the CPU is stuck

you will notice this:

2276 (30.1%) 47 (0.6%) block in Ancestry::ClassMethods#arrange_nodes

for example

Jason Frey
@Fryguy
Sep 07 2016 15:56
right...I was ust going to point that one out
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:56
most of those samples are all from roughly the same stack trace
Jason Frey
@Fryguy
Sep 07 2016 15:56
because coinicdentally I wrote this ages ago ;) stefankroes/ancestry#261
but I don't think that commit was ever released (i.e. we are not using it)
Oleg Barenboim
@chessbyte
Sep 07 2016 15:57
btw - @kbrock and @NickLaMuro -- you guys are doing a great job in dealing with our performance issues across the product
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:57
@Fryguy hah, nice
Jason Frey
@Fryguy
Sep 07 2016 15:57
so we can probably speed that up by releasing a new ancestry or forking or using a git based repo or something
Nick LaMuro
@NickLaMuro
Sep 07 2016 15:58
let me dive in and see if that code is there
Jason Frey
@Fryguy
Sep 07 2016 15:58
I'm pretty sure it's not :/
Keenan Brock
@kbrock
Sep 07 2016 15:58
I'm not sure if that fix helps us. but yes, we need to release it either way
Oleg Barenboim
@chessbyte
Sep 07 2016 15:58
@kbrock merged that PR -- maybe he can convince the author to make a new release of ancestry gem
Jason Frey
@Fryguy
Sep 07 2016 15:59
@kbrock Yeah, you are mostly focusing on scopes flowing through...that was more of a reaction to the stackprof dump
which may or may not be a "real" issue...not sure
Keenan Brock
@kbrock
Sep 07 2016 15:59

ok, my second question: for child_ids, descendant_ids, we always translate to just id and we use :of_type always

Can we change that method?

aside: ancestry gem is old / not maintained
I know we can put some $$ into it but... it isn't really meeting our needs either
s/$$/sweat, time, love/
Jason Frey
@Fryguy
Sep 07 2016 16:02
relationship_mixin is about heterogeneous trees...child_ids should return heterogeneous entities (i.e. type/id pairs)
I guess what I'm missing is what's making that a hotspot
even if we bring back the type and then throw it away...it is really that expensive?
Nick LaMuro
@NickLaMuro
Sep 07 2016 16:03
This message was deleted
Jason Frey
@Fryguy
Sep 07 2016 16:03
if so, I could see a separate method like child_ids_of_type(EmsFolder) that is optimized or something
but it feels clunky from an API perspective
Oleg Barenboim
@chessbyte
Sep 07 2016 16:04
I think what @Fryguy is concerned about is keeping Relationship Mixin like a gem
in that the API should be bigger than our current usage
Keenan Brock
@kbrock
Sep 07 2016 16:05
ok, but that 'optimization' downloads 1 number instead of every record in 2 trees
Jason Frey
@Fryguy
Sep 07 2016 16:05
why does it have to go to a separate table to get the type
we have the resource_type column right in the table
Keenan Brock
@kbrock
Sep 07 2016 16:05
and when I read child_ids I expect integer ids
lol. I didn't write the code ;)
Jason Frey
@Fryguy
Sep 07 2016 16:06
yeah, I see what you mean, but if I have multiple types as children how else would you represent it
Keenan Brock
@kbrock
Sep 07 2016 16:06
100% agreed
that is why I am asking instead of just making the change
Jason Frey
@Fryguy
Sep 07 2016 16:06
I guess I coud accept a second option... :ids_only => true or something
or :include_types => false ¯\_(ツ)_/¯ (naming is hard :P )
Keenan Brock
@kbrock
Sep 07 2016 16:07
our current pattern is resource_pairs_to_ids(child_ids(:of_type => X))
Jason Frey
@Fryguy
Sep 07 2016 16:07
that's a callers concern
or rather...the of_type part is the caller
Keenan Brock
@kbrock
Sep 07 2016 16:07
I like the of_type
it is nice
Jason Frey
@Fryguy
Sep 07 2016 16:07
does the caller call resource_pairs_to_ids?
Keenan Brock
@kbrock
Sep 07 2016 16:08
yes
every time
Jason Frey
@Fryguy
Sep 07 2016 16:08
ah ok...so that's where a second option would make more sense
Keenan Brock
@kbrock
Sep 07 2016 16:08
(minus 1 that is very bad)
Jason Frey
@Fryguy
Sep 07 2016 16:08
I thought resource_pairs_to_ids was inside the child_ids method somehow
Keenan Brock
@kbrock
Sep 07 2016 16:08
i was thinking a method child_ids_but_i_really_just_want_ids
Jason Frey
@Fryguy
Sep 07 2016 16:09

So, I'm thinking instead of

resource_pairs_to_ids(child_ids(:of_type => X))

then have

child_ids(:of_type => X, :include_types => false)

then you can optimize that combination internally to child_ids
Keenan Brock
@kbrock
Sep 07 2016 16:09
I was thinking child_ids and child_class_and_ids
but the other is good too
Jason Frey
@Fryguy
Sep 07 2016 16:10
@NickLaMuro You said you had a blobby answer? :)
Keenan Brock
@kbrock
Sep 07 2016 16:10
:)
Nick LaMuro
@NickLaMuro
Sep 07 2016 16:10
@Fryguy Doing it as a PM at this point, to not derail the convo
Jason Frey
@Fryguy
Sep 07 2016 16:10
haha nice
Nick LaMuro
@NickLaMuro
Sep 07 2016 16:10
carry on
Jason Frey
@Fryguy
Sep 07 2016 16:11
I'm thinking if we remove the remote table access though and child_ids just used the resource_type, then you get all the performance benefits and no API change
Keenan Brock
@kbrock
Sep 07 2016 16:11
actually - you can have the floor
sounds good
Jason Frey
@Fryguy
Sep 07 2016 16:11
I guess the only downside to that is you don't get the STI type, but that can be the caller's concern, I think
Keenan Brock
@kbrock
Sep 07 2016 16:11
child_ids(:of_type => X, :include_types => false) :+1:
if they said "no" then they get what they deserve
Jason Frey
@Fryguy
Sep 07 2016 16:12
:point_up: September 7, 2016 12:11 PM Alternate idea so you don't need the interface change
(to deal with the performance issue)
once the performance issue is dealt with, then the :include_types => false is more cosmetic so you don't have to call resource_pairs_to_ids at the caller side
Keenan Brock
@kbrock
Sep 07 2016 16:13
yes
agreed
same page
Jason Frey
@Fryguy
Sep 07 2016 16:14
So, instead of getting back [[VmVmware, 1], [VmRedhat, 2]] you'd get back [[VmOrTemplate, 1], [VmOrTemplate, 2]] would be the STI downside I mentioned
which is probably not a problem
Keenan Brock
@kbrock
Sep 07 2016 16:15
ooh
well, since we are converting to ids anyway (everywhere) and we pass in :of_type => VmOrTemplate I was thinking about just returning [1, 2]
that way, we can use size and it will just work in sql. (vs the other requires ruby manipulation / downloading everything)
Jason Frey
@Fryguy
Sep 07 2016 16:16
right, but if the focus is performance, then caller can still do resource_pairs_to_ids and there should be no perf loss
Keenan Brock
@kbrock
Sep 07 2016 16:17
I don't want to download 42 (in my case) records, I want to download 1 number
Jason Frey
@Fryguy
Sep 07 2016 16:17
size will still work with the [[VmOrTemplate, 1], [VmOrTemplate, 2]] thing in SQL
because it's within the same table (relationships table)
Keenan Brock
@kbrock
Sep 07 2016 16:18
ok. I'm not sure how that will work, but I'll give it a try.
you are saying something like Relationships.select(:id, :type) - that can still become a size /count. crossing fingers. thanks
Jason Frey
@Fryguy
Sep 07 2016 16:19
more like Relationship.select(:resource_id, :resource_type)
Keenan Brock
@kbrock
Sep 07 2016 16:19
this is 2 PRs away - I'll hit it when I get there
ok
thanks
Jason Frey
@Fryguy
Sep 07 2016 16:19
as opposed to that PLUS query for the actual STI type in all the remote tables, just to throw away the STI types
ok lunch time :fries: :)
Keenan Brock
@kbrock
Sep 07 2016 18:04
oh my, so it is 161s (2.75 minutes) to load vms explorer with this db
Dennis Metzger
@dmetzger57
Sep 07 2016 18:08
yea, defintely an area for improvement
Oleg Barenboim
@chessbyte
Sep 07 2016 18:14
these trees are killer
Jason Frey
@Fryguy
Sep 07 2016 18:14
it's the nature of it though...the amount of data is crazy...topology will have the same problem
Oleg Barenboim
@chessbyte
Sep 07 2016 18:15
think the whole concept of preloading, prefetching the whole tree and then showing only what is expanded needs to be reviewed. My gut has always told me it makes no sense to do this computationally
Jason Frey
@Fryguy
Sep 07 2016 18:15
so you either play games to trim it down...change it to load partially on the fly, etc
or you just drop it altogether and use search
Oleg Barenboim
@chessbyte
Sep 07 2016 18:15
I think it should be changed to load/fetch/show ONLY what is needed
Jason Frey
@Fryguy
Sep 07 2016 18:16
agreed, but that is a lot easier said than done when you have to show a partial tree
Oleg Barenboim
@chessbyte
Sep 07 2016 18:16
farfegnuggen
Jason Frey
@Fryguy
Sep 07 2016 18:16
like when a dozen nodes are opened with no rhyme or reason
or you get to a place where the tree is loaded up front and updated with ajax
and you never drop the tree from the page
Oleg Barenboim
@chessbyte
Sep 07 2016 18:17
would love to see what newer UI people on our team have to say about UX and need for trees
Jason Frey
@Fryguy
Sep 07 2016 18:17
so a new VM gets added, renamed, power state change, whatever...ajax just drops it in the tree in the right place
Oleg Barenboim
@chessbyte
Sep 07 2016 18:17
I am a search person myself
Dan Clarizio
@dclarizio
Sep 07 2016 18:18
fyi, we used to load the tree nodes on demand . . . the problem becomes when you leave that screen and come back, the user expects the tree to be the same and it grinds trying to retrace all the nodes
Jason Frey
@Fryguy
Sep 07 2016 18:18
right
in fact most of the trees still do that
but for many trees, due to their shape, it's not as much of a problem
notifications work might go a long way to telling the UI which parts to update
Oleg Barenboim
@chessbyte
Sep 07 2016 18:19
@dclarizio I know that is how we developed it -- would love to see real customer experiences to see if that is how people use our UI
Dan Clarizio
@dclarizio
Sep 07 2016 18:19
we have searchable accordions and the user CAN turn off the tree based ones, but trees show hierarchy that search doesn't . . . and many providers we support have hierarchies that help users navigate
Keenan Brock
@kbrock
Sep 07 2016 18:19
I like just not putting vms into the tree
Jason Frey
@Fryguy
Sep 07 2016 18:19
I still think we'll get huge gains by just not loading collapsed accordions
Keenan Brock
@kbrock
Sep 07 2016 18:20
it takes ~100 seconds off of the page
Jason Frey
@Fryguy
Sep 07 2016 18:20
ops_settings screen especially
Dan Clarizio
@dclarizio
Sep 07 2016 18:20
@kbrock I like that idea too . . . but we need to get consensus as "some" of the trees are mimicking the mgmt tools of the provider, which shows them
Jason Frey
@Fryguy
Sep 07 2016 18:20
@dclarizio Is that still a requirement?
I feel like we've moved on past that, but ¯\_(ツ)_/¯
Dan Clarizio
@dclarizio
Sep 07 2016 18:21
I don't make the requirements, hence the need for consensus from those that do
Oleg Barenboim
@chessbyte
Sep 07 2016 18:21
@dclarizio I honestly think the approach to take is to make the tree optional or a new "type" of view that users have to click on
Jason Frey
@Fryguy
Sep 07 2016 18:21
so let's push to drop that requirement :)
Dan Clarizio
@dclarizio
Sep 07 2016 18:21
many of the tree accordions are optional . . .easily turned off
Jason Frey
@Fryguy
Sep 07 2016 18:21
yeah, but do they still load when off like the accordions do?
Dan Clarizio
@dclarizio
Sep 07 2016 18:22
nope
Oleg Barenboim
@chessbyte
Sep 07 2016 18:22
no - I want the default to be OFF
have users opt-in to see the tree
right now, they have no choice
Jason Frey
@Fryguy
Sep 07 2016 18:22
...like topology is now
Dan Clarizio
@dclarizio
Sep 07 2016 18:22
again, I'd only do that with info that shows users won't complain
Oleg Barenboim
@chessbyte
Sep 07 2016 18:23
why would they complain if we give them topology/tree with a button on upper right?
Dan Clarizio
@dclarizio
Sep 07 2016 18:23
I think it would be on an area by area basis . . . in some areas, the trees are needed I think
Oleg Barenboim
@chessbyte
Sep 07 2016 18:23
I disagree
Jason Frey
@Fryguy
Sep 07 2016 18:23
maybe like automate or policy it's still needed
because you can't navigate otherwise
Oleg Barenboim
@chessbyte
Sep 07 2016 18:23
why can't you navigate otherwise?
I guess that is my fundamental question
Jason Frey
@Fryguy
Sep 07 2016 18:24
because there isn't any navigation for them at the moment
those should have search or something
Oleg Barenboim
@chessbyte
Sep 07 2016 18:24
why not have a list/grid/tile/tree/topology view?
Dan Clarizio
@dclarizio
Sep 07 2016 18:24
see, I hate search based . . . I use it sometimes, but prefer to visually see things
Jason Frey
@Fryguy
Sep 07 2016 18:24
yeah, it would have to be added
Oleg Barenboim
@chessbyte
Sep 07 2016 18:24
btw, does tree == topology?
Dan Clarizio
@dclarizio
Sep 07 2016 18:24
opposite of @chessbyte
Jason Frey
@Fryguy
Sep 07 2016 18:24
I prefer visual too
but search is your access to visual
for example, If I know the folder path, I can search for in_path:"/Vms/My VMs"
Dan Clarizio
@dclarizio
Sep 07 2016 18:25
right, I search for a piece of code, but I like to see where it is in the folder structure in a tree
Jason Frey
@Fryguy
Sep 07 2016 18:25
right, but do you still need everything else open?
Nick LaMuro
@NickLaMuro
Sep 07 2016 18:25
@dclarizio @Fryguy I was going to suggest would a github file folder structure make more sense
Dan Clarizio
@dclarizio
Sep 07 2016 18:26
I only open what I need
Nick LaMuro
@NickLaMuro
Sep 07 2016 18:26
ala: https://github.com/ManageIQ/manageiq/tree/master/app/models (the route preserves location, and can be navigated back to, but only loads what it needs (oh well... I tried))
Jason Frey
@Fryguy
Sep 07 2016 18:26
@dclarizio I think that comes back to my "always on" tree idea from up above
Dan Clarizio
@dclarizio
Sep 07 2016 18:26
hence, the fetch when opening DOES make sense, but performance wise, it has certain challenges
Jason Frey
@Fryguy
Sep 07 2016 18:26
Like in Sublime I keep the tree open
Dan Clarizio
@dclarizio
Sep 07 2016 18:26
I use trees for a lot of things
email
files
code
Jason Frey
@Fryguy
Sep 07 2016 18:27
right, but in those things it doesn't reload the tree every click
Dan Clarizio
@dclarizio
Sep 07 2016 18:27
I think in file explorers, it discovers when it opens a node
Jason Frey
@Fryguy
Sep 07 2016 18:27
if we kept the tree "always open", then it just needs to be dynamically updated
@dclarizio depends...it might slow load in the background
Dan Clarizio
@dclarizio
Sep 07 2016 18:28
yeah, there is that as well
Jason Frey
@Fryguy
Sep 07 2016 18:28
but it also dynamically updates...if I touch a file in a terminal it just appears in my explorer iew
Dan Clarizio
@dclarizio
Sep 07 2016 18:28
slow load
yes, id does dynamically update, but the data is local . . . perfomance hits it if you're perusing a distant network drive
Jason Frey
@Fryguy
Sep 07 2016 18:28
right
those things can also handles lots-of-files directories
if you created a 10000 VMs flat tree, our explorers just die
Dan Clarizio
@dclarizio
Sep 07 2016 18:30
yep, cuz we're in a browser, so that's even more limiting
Oleg Barenboim
@chessbyte
Sep 07 2016 18:30
but is that what people do -- a 10k VMs flat tree?
Jason Frey
@Fryguy
Sep 07 2016 18:30
yeah, so we probably need something at the browser level that can tolerate that better
Dan Clarizio
@dclarizio
Sep 07 2016 18:30
yeah, I like not showing the VMs in the tree . . . buys us a LOT
we should push for that
Oleg Barenboim
@chessbyte
Sep 07 2016 18:31
GitHub File Folder sounds like an interesting option
Jason Frey
@Fryguy
Sep 07 2016 18:31
Dicovered Virtual Machines blue folder many times is just a flat set of VMs
Dennis Metzger
@dmetzger57
Sep 07 2016 18:31
+1 for no VMs
Jason Frey
@Fryguy
Sep 07 2016 18:31
I'm actually +1 on no VMs as well
Oleg Barenboim
@chessbyte
Sep 07 2016 18:31
ok - so, write up a proposal for John/Brad and let's get them to buy in
Dan Clarizio
@dclarizio
Sep 07 2016 18:32
will be some changes to how the tree operates, but should be simple and straightforward
Jason Frey
@Fryguy
Sep 07 2016 18:32
yeah, one interesting thing would be instead of highlighting a vm, you'd have to highlight it's parent folder (or resource_pool or host or something)
Dan Clarizio
@dclarizio
Sep 07 2016 18:33
right, that's one change
oh man, I can close a TON of duplicate RFEs for changing the tree node state icons!!!
maybe half of the outstanding RFEs :laughing:
Keenan Brock
@kbrock
Sep 07 2016 20:47
ok, posted numbers on ManageIQ/manageiq#11003 looks like not putting VMs into navbar takes page rendering time from 161s to 10s
and I think I can shave another 1-2s off of that
as mentioned before, 22s is on the server, but there is so much data that it takes an extra 2 minutes to render the explorer tree
also of note, if you know the spot in the tree, the navbar is quite usable
Jason Frey
@Fryguy
Sep 07 2016 20:55
ah right, so this knocks off mostly the rendering time
what is the render vs database split in the times before/after
Keenan Brock
@kbrock
Sep 07 2016 20:55
well, 1/3 the server rendering time too
render 161s -> 10s
server 22s -> 6.7s
70% faster server side
Jason Frey
@Fryguy
Sep 07 2016 20:56
cool
do you have client side numbers?
Keenan Brock
@kbrock
Sep 07 2016 20:56
that is 22s and 161s
Jason Frey
@Fryguy
Sep 07 2016 20:56
i.e. the time it takes to actually draw the tree with dynatree
Keenan Brock
@kbrock
Sep 07 2016 20:56
that was taken from the browser
the numbers are a little off
it kept crashing the browser on be for the before numbers
it is somewhere from 154 to 166
I had chrome say "um, no. I'm not displaying this" a few times
to be fair - it is 2.6 minutes
Jason Frey
@Fryguy
Sep 07 2016 20:57
ah ok...i thought those two numbers were coming from the log line
in development.log production.log
Keenan Brock
@kbrock
Sep 07 2016 20:57
production mode
3rd render
as admin
render is time spent in the browser to build the tree
suggestions on wording to make more clear?
and it is actually the time to build the whole page. rack mini profiler times it until page ready dom event
the page is 1g of memory too
Dan Clarizio
@dclarizio
Sep 07 2016 21:01
need to verify the navigation as well . . . from what I recall, it used to actually issue a tree select when clicking on an item on the right . . . this way, the tree was properly set prior to fetching the item for the summary screen . . . may need slight change there
Keenan Brock
@kbrock
Sep 07 2016 21:02
I was able to click on them
on another pr, wasn't getting correct expanded on the server
rebooting - nick says the ui is not as slow as it is on my mac. maybe the 161s is inflated. think the 1g required to render the page is hitting swap
Dan Clarizio
@dclarizio
Sep 07 2016 21:07
@kbrock good to know . . . but will for sure have to check navigation to this page from outside pages and such . . . and, of course, get approval for the change, unless we make it optional somehow
Keenan Brock
@kbrock
Sep 07 2016 21:18
that change is optional
need a setting to turn it on
@dclarizio I'd prefer customers to flip the "we are big" switch rather than have us run counts on every page
Dan Clarizio
@dclarizio
Sep 07 2016 21:19
you mean "we are small" switch . . . so they'd have to turn it on
Keenan Brock
@kbrock
Sep 07 2016 21:19
ooh, I did "we are big" switch, but yea. I can do that if you want
Dan Clarizio
@dclarizio
Sep 07 2016 21:20
just that the default is we are big, no VMs in the tree . . . turn on I don't care to get them . . . but we'll see, maybe we can toss them completely
Nick LaMuro
@NickLaMuro
Sep 07 2016 22:47

@dclarizio After doing a git bisect, it looks like this PR caused the slowness in the UI: ManageIQ/manageiq#10767

Specifically this commit: ManageIQ/manageiq@973a1dd

This is in reference to what @kbrock was talking about with the UI slowness earlier:

@kbrock:
that was taken from the browser
the numbers are a little off
it kept crashing the browser on be for the before numbers
it is somewhere from 154 to 166
I had chrome say "um, no. I'm not displaying this" a few times
to be fair - it is 2.6 minutes