These are chat archives for ManageIQ/manageiq/performance

2nd
Jun 2016
Keenan Brock
@kbrock
Jun 02 2016 14:41
@jrafanie there is a metadata function that looks up some ids, and with those ids counts data in the hardware table?
does this ring a bell?
wanted to do talk on this method and improving it. it is used in ~10+ classes
can't remember what it was though :(
thanks. explaining it helped me realize it was aggregate_hardware
Joe Rafaniello
@jrafanie
Jun 02 2016 14:46
@kbrock :sweat_smile: I had no idea
Jason Frey
@Fryguy
Jun 02 2016 14:47
See aggregation_mixin
Keenan Brock
@kbrock
Jun 02 2016 14:47
thanks
Jason Frey
@Fryguy
Jun 02 2016 14:47
Definitely needs improvement
Keenan Brock
@kbrock
Jun 02 2016 14:47
it is the perfect one for a talk
Jason Frey
@Fryguy
Jun 02 2016 14:47
But would weigh that against other possible wins
Ah for the talk...cool
Keenan Brock
@kbrock
Jun 02 2016 14:48
if you have ideas of stuff like this that are as straight forward, but actually used... that would be great
anything that used preload or gets ids and works on them
Joe Rafaniello
@jrafanie
Jun 02 2016 15:09
@akrzos @dmetzger57 lazy loading po's are finally ready to go: ManageIQ/manageiq#8525
:tada:
Dennis Metzger
@dmetzger57
Jun 02 2016 15:11
:clap:
Keenan Brock
@kbrock
Jun 02 2016 15:13
@jrafanie I think we get hit by that on every page request set_gettext_locale takes 32ms
(I had a BZ that said we were hitting the server on every keypress and went from 56ms => 400ms)
so that 32 (1/2 of our allocation) is quite high
I'm looking forward to seeing the improvements
Joe Rafaniello
@jrafanie
Jun 02 2016 15:14
I'm not sure why we'd set the locale more than 1 place: where we allow you to configure it
Keenan Brock
@kbrock
Jun 02 2016 15:14
but we need to look up the value configured
and convert into _t
or what ever
since every page displays localized data, every page needs to get a reference to the approperiate object
Joe Rafaniello
@jrafanie
Jun 02 2016 15:15

look up the value configured

I wouldn't think "look up" would mean set_gettext_locale

maybe get_gettext_locale
Keenan Brock
@kbrock
Jun 02 2016 15:15
I just work here
ooh, and get_timezone is also 30ms - those two are doozies and on every page request
huh
so on that note, we set the user's timezone in the session
is there a reason we are setting that in the session on EVERY request?
Joe Rafaniello
@jrafanie
Jun 02 2016 15:17
you have to sift through all the noise and figure out which ones are important
Keenan Brock
@kbrock
Jun 02 2016 15:17
those 2 are the most expensive filters on every request
and I have a BZ that says the ui is jumpy because of those 2
Joe Rafaniello
@jrafanie
Jun 02 2016 15:18
try hardcoding those things and see if it helps... measure, measure, measure
Keenan Brock
@kbrock
Jun 02 2016 15:18
I did hard code it
looking up the server timezone speeds it up
Joe Rafaniello
@jrafanie
Jun 02 2016 15:18
if hardcoding helps significantly, then ask why to people who may know
Keenan Brock
@kbrock
Jun 02 2016 15:18
joe....
I created a PR and jason wanted it done another way... think we're almost there
but the bigger question is why we are setting these on every request
no code > fast code
Joe Rafaniello
@jrafanie
Jun 02 2016 15:19
:+1: I agree
I would think you could keep it in session and only change it if they change it
accessing the timezone out of session should not be slow
Keenan Brock
@kbrock
Jun 02 2016 15:20
tracking down history in application_controller is overwhelming
Joe Rafaniello
@jrafanie
Jun 02 2016 15:21
what part is slow? setting or getting timezone in session?
Keenan Brock
@kbrock
Jun 02 2016 15:21
reading from config
Jason Frey
@Fryguy
Jun 02 2016 15:21
@kbrock Did you try my PR?
(I know tests are failing, but I'm pretty sure that's test only)
Joe Rafaniello
@jrafanie
Jun 02 2016 15:21
why are we reading from config if we have it in session?
Keenan Brock
@kbrock
Jun 02 2016 15:21
ok
Jason Frey
@Fryguy
Jun 02 2016 15:22
there are two timezone values, btw
server timezone and user timezone
Joe Rafaniello
@jrafanie
Jun 02 2016 15:22
Maybe when you save the config is where you change the value in session
Keenan Brock
@kbrock
Jun 02 2016 15:22
original thread, set_gettext_local is slow, matters, and I hope it will be improved by lj's change
Jason Frey
@Fryguy
Jun 02 2016 15:22
and user locale != user timezone
just want to make sure we are not mixing things when discussing
Keenan Brock
@kbrock
Jun 02 2016 15:22
yes, tangent of tangent
@Fryguy
MiqServer.my_server.get_config("vmdb").config.fetch_path(:server, :locale)
what should that code be? use Settings?
@Fryguy thanks for reminder of PR, I did test the other area and it was faster. will try and remember to test this method as well
Joe Rafaniello
@jrafanie
Jun 02 2016 15:29
ooh, low hanging fruit... @dmetzger57 @akrzos @Fryguy that sprockets stat'ing of the assets on rails boot appears to incur ~12 MB of memory on rails boot
Joe Rafaniello
@jrafanie
Jun 02 2016 15:35
^ gist
Joe Rafaniello
@jrafanie
Jun 02 2016 15:59
Grabbing lunch, @akrzos @dmetzger57 backporting rails/sprockets#211 from that gist to a forked sprockets or continue to beg for a 3.6.x release with this is the next easy win
Dennis Metzger
@dmetzger57
Jun 02 2016 16:00
lunch, I’m starving over here…..
Keenan Brock
@kbrock
Jun 02 2016 17:44
@Fryguy @jrafanie What is behind all_host_ids? that means recursivly figure them out? Distinguish from direct values?
(there are tons of all_*_ids)
Jason Frey
@Fryguy
Jun 02 2016 17:45
I believe so, but can you give me a link to verify?
Oh...you're in AssigmentMixin...yeah, that's the default implementation...mixees can change the implementation for something more performant
Keenan Brock
@kbrock
Jun 02 2016 17:47
so that is the only code that uses all_*_ids?
I also noticed a few places do stuff like direct_vms + indirect_vms (I really don't like this pattern)
but that may be totally unrelated
@Fryguy do we use this code that much? don't all reports use it?
Jason Frey
@Fryguy
Jun 02 2016 17:48
that's a good question on who uses all_host_ids...It might only be aggregation_mixin, but I'm not really sure
you'd have to look at the mixees
Keenan Brock
@kbrock
Jun 02 2016 17:49
perfect - thanks
grep all_.*ids ;)
Jason Frey
@Fryguy
Jun 02 2016 17:49

do we use this code that much? don't all reports use it?

which are you referring to ... all_*_ids or direct_vms+indirect_vms

Keenan Brock
@kbrock
Jun 02 2016 17:49
I have slowly been removing the direct + indirect
just noticed it is in ems_cluster
:(
Jason Frey
@Fryguy
Jun 02 2016 17:52
I kind of liked the :s face :)
Keenan Brock
@kbrock
Jun 02 2016 17:52
lol

ContainerManager:

    def all_computer_system_ids ##
      MiqPreloader.preload(container_nodes, :computer_system)
      container_nodes.collect { |n| n.computer_system.id }
    end

is an optimization?

lol
well, at least I won't lack for content for my talk
Jason Frey
@Fryguy
Jun 02 2016 17:57
not sure on that one
we do preload in the method like that in a number of places because container_nodes or whatever the method is may be filtering
Keenan Brock
@kbrock
Jun 02 2016 18:14
sorry, was complaining.
A better way to respond would be to submit a PR
Jason Frey
@Fryguy
Jun 02 2016 21:34
Also, with @jrafanie's help we made a breakthrough and have some parallel processing of the conversion, so that might speed it up as well
@akrzos we may need a bit of back and forth to find the magic batching size though, so I might need your help with that
Keenan Brock
@kbrock
Jun 02 2016 22:03
@Fryguy we care that we are putting an MiqReport object onto the queue? ==> async.rb
Jason Frey
@Fryguy
Jun 02 2016 22:10
We probably shouldn't be, unless it's not saved ¯\_(ツ)_/¯
Keenan Brock
@kbrock
Jun 02 2016 22:10
not saved
Jason Frey
@Fryguy
Jun 02 2016 22:10
yeah, that's probably the reason....hmmm
not sure what to do about that
Keenan Brock
@kbrock
Jun 02 2016 22:10
I had just cc/d you on the pr ==> ManageIQ/manageiq#8963
I put in a pr for the "test" - although, that won't really affect us
punt seems like a viable response... just sayin'
@gtanzillo FYI -- updated and this passed specs: ManageIQ/manageiq#9001