These are chat archives for ManageIQ/manageiq/performance

20th
Apr 2016
Keenan Brock
@kbrock
Apr 20 2016 18:48
Man, this Relationship model is very suspect. Sure causes a lot of stuff to be loaded into memory
Oleg Barenboim
@chessbyte
Apr 20 2016 18:52
@kbrock - what shiny object are you chasing? :-)
there is a method that I dislike - really hits us for performance
AND that method just threw an exception for a customer
crux: for an ems, get storages, for those storages put in queue
but part 2 has to lookup every ems to find a queue name
only works as N+1
but that is the SHINY part of it
Oleg Barenboim
@chessbyte
Apr 20 2016 18:53
oh yes - I know that bit of code
Keenan Brock
@kbrock
Apr 20 2016 18:54
it is in my top 10 list
Oleg Barenboim
@chessbyte
Apr 20 2016 18:54
ooh - please share your top 10 list
Keenan Brock
@kbrock
Apr 20 2016 18:54
I need to write these down
anyway
Oleg Barenboim
@chessbyte
Apr 20 2016 18:54
LOL
Keenan Brock
@kbrock
Apr 20 2016 18:54
don't tempt me
eject
I'm guessing this is blowing up because it can't guess the ems from a storage
Oleg Barenboim
@chessbyte
Apr 20 2016 18:54
actually, there is NOTHING wrong with documenting your optimization ideas somewhere
Keenan Brock
@kbrock
Apr 20 2016 18:55
for you and me both!
Oleg Barenboim
@chessbyte
Apr 20 2016 18:55
would only help Dennis, Jason and me
Keenan Brock
@kbrock
Apr 20 2016 18:55
(and me)
Jason Frey
@Fryguy
Apr 20 2016 18:55

can't guess the ems from a storage

storages can cross multiple EMSes

Keenan Brock
@kbrock
Apr 20 2016 18:55
I'm starting to feel like the ui. if there is a bug in code and it causes performance - it is on my plate :)
Jason Frey
@Fryguy
Apr 20 2016 18:55
(I think)
Keenan Brock
@kbrock
Apr 20 2016 18:55
not multiple ems vendors though
it just needs a queue name
also, adding a method that asks for the class and acts accordingly. would have been nice if we just defined it in the classes
but again - that is shiny
@fryguy - verify: metrics_collector_queue_name is per ems.type vendor NOT per instance of ems?
I can track it down I guess
in general, metrics_collector_queue_name is a trouble spot for performance.
and maybe it is just a little bad, but it seems like it doesn't need to be
Keenan Brock
@kbrock
Apr 20 2016 19:00
Oleg. you and I generated 2 last night:
  • deprecate_attribute [loads class at boot] need to rework virtual_attribute
  • rails rails/rails#24653 [ 7% performance boost ]
Joe Rafaniello
@jrafanie
Apr 20 2016 19:04
Ooh, when do we see the benefits of that 7%?
Keenan Brock
@kbrock
Apr 20 2016 19:04
I have to write a test. looks like they are optimistic
7% = running test suite
Joe Rafaniello
@jrafanie
Apr 20 2016 19:04
Once at rails load?
Keenan Brock
@kbrock
Apr 20 2016 19:04
everyone / travis runs
rails load
yea
local specs
Joe Rafaniello
@jrafanie
Apr 20 2016 19:05
Yay
Keenan Brock
@kbrock
Apr 20 2016 19:05
I only tested on running 1 model spec and all model specs
it is double defining our classes (load_schema - defining virtual attributes ...)
@Fryguy / @jrafanie what do you think about defining a local @first_ems variable in storages?
so if you want to say "just give me an ems please" it can use that
instead of storage.ext_management_systems.first
that would speed up C&U like 10%?
well, IF they have storages
Jason Frey
@Fryguy
Apr 20 2016 19:16
how can you only do C&U on 1 storage on an EMS?
Keenan Brock
@kbrock
Apr 20 2016 19:16
when it is trying to queue a storage request, it needs to just pick ANY ems to get the queue name
so it stores the name of "any ems"
typically when the storage object in memory was created FROM an ems
Jason Frey
@Fryguy
Apr 20 2016 19:18
what is the EMS for though? to get the zone?
Keenan Brock
@kbrock
Apr 20 2016 19:18
to get the queue name
Oleg Barenboim
@chessbyte
Apr 20 2016 19:19
like ems_1?
Keenan Brock
@kbrock
Apr 20 2016 19:19
  def queue_name_for_metrics_collection
    ems = if self.kind_of?(ExtManagementSystem)
            self
          elsif self.kind_of?(Stroage)
            ext_management_systems.first
          elsif self.respond_to?(:ext_management_system)
            ext_management_system
          else
            raise _("Unsupported type %{name} (id: %{number})") % {:name => self.class.name, :number => id}
          end

    ems.metrics_collector_queue_name
  end
it is in a few places of the code
would be nice if either the class could derive the queue name from it's own class (not use ems)
OR it rembered the name of any ems
Joe Rafaniello
@jrafanie
Apr 20 2016 19:31
did you type that or is it a typo in the code?
elsif self.kind_of?(Stroage)
Keenan Brock
@kbrock
Apr 20 2016 19:31
sorry
I delete the line and pasted back in
Joe Rafaniello
@jrafanie
Apr 20 2016 19:31
haha
good
it seems like this is one of those cases where a constant/class ivar could hold all the different queue types and each provider could register their queue name into it... as it is now, it jumps through hoops to get that information and it's over factored, at least for the purpose of this problem
Keenan Brock
@kbrock
Apr 20 2016 20:09
@jrafanie the reason why I mention it, you get all the storages, and then for each of them it loads all the ems to pick 1
10 ems -> 100 storages
loads 10 + 100 * 10 == 1010 ems
Joe Rafaniello
@jrafanie
Apr 20 2016 20:14
does it load all of them, isn't there a shortcut if first isn't it? like take(1) or something
Keenan Brock
@kbrock
Apr 20 2016 20:15
class Storage
  def ext_management_systems
    @ext_management_systems ||= ExtManagementSystem.joins(:hosts => :storages).where(
      :host_storages => {:storage_id => id}).distinct.to_a
  end
end
Joe Rafaniello
@jrafanie
Apr 20 2016 20:17
oh, that
:cry:
well, at least we cache it ;-)
Keenan Brock
@kbrock
Apr 20 2016 20:22
lol
Jason Frey
@Fryguy
Apr 20 2016 20:30
o_O
ugh I thought it was a normal HABTM
or rather a has_many :through
can we just change that to has_many :ext_management_systems, :through => :hosts?
Keenan Brock
@kbrock
Apr 20 2016 20:32
I guess so
I was hoping to avoid the lookup in the first place
Jason Frey
@Fryguy
Apr 20 2016 20:32
well at least if it's a normal scope, then you can do a .limit(1) on it to get the first and still have a scope
Keenan Brock
@kbrock
Apr 20 2016 20:32
there is a distinct there
Jason Frey
@Fryguy
Apr 20 2016 20:32
I think has many through handles disctintness
if not, it's an option I'm sure
haven't run numbers yet. need to get cap and u back up and running
I did a lot of 2 way association things
(inverse_of)
and here that is what it felt like we wanted
but won't work because it is not going to assign just one to the association
... but that is exactly what we want...

curious: https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/base_manager/refresher.rb#L33-L42

the first line says only take records with an "EMS", but the later says, maybe we called the ems a manager
Surprised refreshes are working at all for any model with an manager (e.g.: foreman)