These are chat archives for ManageIQ/manageiq/performance

2nd
Sep 2016
Jason Frey
@Fryguy
Sep 02 2016 17:44
@kbrock reading back, but I don't understand why ext_management_system would need that kind of mixing...it has normal STI
The only reason you need this mixin for Template and VM is because that is a separation layer in the STI
Keenan Brock
@kbrock
Sep 02 2016 17:52
@Fryguy we want all orphans for infra
Jason Frey
@Fryguy
Sep 02 2016 17:52
Yeah, I get the requirements
In a way this is good we never physically split vms and templates, right? :trollface:
Keenan Brock
@kbrock
Sep 02 2016 17:53

why ext_management_system would need that kind of mixing

trying to parse / understand that sentance

yea
I can keep them separate
Jason Frey
@Fryguy
Sep 02 2016 17:53
mixing like (Vm.descendants + Tempalte.descendants)
So you had said there were a handful of UI places that do that but for EMS
and I can't understand why it would ever be needed
Keenan Brock
@kbrock
Sep 02 2016 17:54
ooh
no
don't think for ems
Jason Frey
@Fryguy
Sep 02 2016 17:54
Keenan Brock
@kbrock
Sep 02 2016 17:55
ooh, all those classes mixed descendants but for vm/template lookup
Jason Frey
@Fryguy
Sep 02 2016 17:55
or a little higher up :point_up: September 1, 2016 2:14 PM
can you link to an example one? Just trying to understand the UI use case
Keenan Brock
@kbrock
Sep 02 2016 17:55
I just grepped \bsubclasses\.
Jason Frey
@Fryguy
Sep 02 2016 17:56
oh i see...I thought you were saying they called that on EMS
Keenan Brock
@kbrock
Sep 02 2016 17:57
mostly on vm / template
Jason Frey
@Fryguy
Sep 02 2016 17:57
ugh...found this one too, but it seems this can just use an operator right? return [] unless OrchestrationTemplate.subclasses.collect(&:name).include?(template_type.to_s)
Keenan Brock
@kbrock
Sep 02 2016 17:58
yea, I was just focusing on the Vm.map(&:name) + Template.map(&:name)
Jason Frey
@Fryguy
Sep 02 2016 17:58
gotcha
Keenan Brock
@kbrock
Sep 02 2016 17:58
and to be honest
besides being very verbose
and forcing us into queries instead of relations
it is the + that I don't like
Jason Frey
@Fryguy
Sep 02 2016 17:58
yeah I agree 100%
love the idea in the PR to have a central place for asking this quetion and then scoping it
Keenan Brock
@kbrock
Sep 02 2016 17:58
well, + for the subclass names is ok
but + on the results - that foces all sorts of queries and rbac stuff
Jason Frey
@Fryguy
Sep 02 2016 17:59
I'm just not happy with it being an AR subclass itself...feel like theres another less confusing construct we could use
Keenan Brock
@kbrock
Sep 02 2016 17:59
yea, don't like the same things you don't lke
Jason Frey
@Fryguy
Sep 02 2016 17:59
but high level, it's awesome
and you'll get some nice reuse in those places in the UI
Keenan Brock
@kbrock
Sep 02 2016 17:59
AAArM? ;)
Jason Frey
@Fryguy
Sep 02 2016 17:59
LOLOL
Keenan Brock
@kbrock
Sep 02 2016 17:59
AAArScope
Jason Frey
@Fryguy
Sep 02 2016 17:59
oh maybe
Keenan Brock
@kbrock
Sep 02 2016 17:59
huh
Jason Frey
@Fryguy
Sep 02 2016 18:00
actually...that's not a bad idea....hmmm
got a meeting, but let's chat afterwards
Keenan Brock
@kbrock
Sep 02 2016 18:00
+1
sometimes I feel like we're developing in java com.manageiq.providers.inframanager.vm
(25% snark)
Jason Frey
@Fryguy
Sep 02 2016 18:17
organizing code is hard :P
Keenan Brock
@kbrock
Sep 02 2016 18:17
cleaning up after the meal is very hard too
( s /meal/spagetti/, s/spagetti/code/)
nice, so I'm deleting this comment:
        # TODO: potential to move this into a model with a scope built into it
Keenan Brock
@kbrock
Sep 02 2016 20:56
@Fryguy Thanks for help with ManageIQ/manageiq#10961 - made changes
You need me to split up ManageIQ/manageiq#10717 ?
Jason Frey
@Fryguy
Sep 02 2016 20:59
@kbrock Still have open comments that weren't addressed (did you forget to push)?
Keenan Brock
@kbrock
Sep 02 2016 20:59
ooh. I'll refresh the browser. thanks
aah - I made those changes to the other class. thanks
Jason Frey
@Fryguy
Sep 02 2016 21:04
I love that AAAScope solved my problem haha
I know you want to kill those classes, but they are just so damn useful :P
Keenan Brock
@kbrock
Sep 02 2016 21:04
+1
now that we have scopes going through rbac, don't have a vendetta anymore
Jason Frey
@Fryguy
Sep 02 2016 21:04
haha cool
Keenan Brock
@kbrock
Sep 02 2016 21:05
ooh. and. a spec would have caught this bug :(
Nick LaMuro
@NickLaMuro
Sep 02 2016 21:07
soooooo.... basically you made a query object...
Keenan Brock
@kbrock
Sep 02 2016 21:07
ha!
<3
Nick LaMuro
@NickLaMuro
Sep 02 2016 21:07
:point_up: September 1, 2016 1:15 PM (just saying)
:D
Jason Frey
@Fryguy
Sep 02 2016 21:09
haha
Keenan Brock
@kbrock
Sep 02 2016 21:11
heh
look ma. I use ALL design patterns in my code
GoF would be jealous
Jason Frey
@Fryguy
Sep 02 2016 21:12
I mean you did say we were getting very Java-like :P
Keenan Brock
@kbrock
Sep 02 2016 21:12
ha!

re ManageIQ/manageiq#10717 - there are 3 bug fixes in there.
I can try and pull apart. It is tricky with the way the original test code was written

I need delegates for a bunch of performance fixes, and need those PRs
So anything you can think that will make your life easier for merging, just let me know

(those PRs = fix delegates and make delegates more powerful)
Jason Frey
@Fryguy
Sep 02 2016 21:25
since we're talking design patterns I'm going to post one of my favorite articles ever: http://blog.plover.com/prog/design-patterns.html
Jason Frey
@Fryguy
Sep 02 2016 21:38
I reread that article and the most recent example of something from the Ruby language that does this was the whole alias_method_chain pattern. This was "solved" by improving Ruby to have the .prepend method and using super. The pattern showed a deficiency in Ruby, so they changed the language to build it in. Now, since prepend is built-in, alias_method_chain is considered an anti-pattern
Keenan Brock
@kbrock
Sep 02 2016 21:39
nice
Keenan Brock
@kbrock
Sep 02 2016 22:00
@Fryguy omfg. I added the documentation. yes. that method with only 6 lines is complicated
Jason Frey
@Fryguy
Sep 02 2016 22:01
haha
Keenan Brock
@kbrock
Sep 02 2016 22:01
if the method itself has 1/2 the lines with a comment above them
warning
if the specs added for modifying 3 lines in there adds 150 lines - warning
ooh
if you have a bug in mind, and you tell matthewd and he says "sorry, that bug can't happen" - warning

there are 3 use cases separated with a ---.
I felt it made more sense to look at them from the class level

So if you can think of a way to reduce, let me know [docs]

Jason Frey
@Fryguy
Sep 02 2016 22:04
out of curiosity what does the _ss stand for?
Keenan Brock
@kbrock
Sep 02 2016 22:04
um
sub select
?
Jason Frey
@Fryguy
Sep 02 2016 22:04
ahhh
Keenan Brock
@kbrock
Sep 02 2016 22:04
I wrote up this PR in december?
maybe
but when asked "reproduce it"
I couldn't
and then in our tests, we use self joins...
BINGO
we don't need to fix for our code, just for our test cases :(
ooooh ss. that is not good
if you have another name, let me know
Jason Frey
@Fryguy
Sep 02 2016 22:06
ohhhhh... I just realized this is all to create a subselect
I thought it was creating a primary query, and couldn't understand why you were aliasing
Keenan Brock
@kbrock
Sep 02 2016 22:06
worse
if you don't tack on the alias, it "runs"
but doesn't work - returns a nil
Jason Frey
@Fryguy
Sep 02 2016 22:06
well, yeah because it collides
Keenan Brock
@kbrock
Sep 02 2016 22:07
yea
obviouse 20/20
Jason Frey
@Fryguy
Sep 02 2016 22:07
but seeing this PR in isolation, I didn't realize its purpose was for subselects
Keenan Brock
@kbrock
Sep 02 2016 22:07
yea
small PRs
the trouble with them
Jason Frey
@Fryguy
Sep 02 2016 22:07
I think even in a big PR I would have missed it :)
Keenan Brock
@kbrock
Sep 02 2016 22:07
the ar_virtual stuff (and the arel)
very very tricky stuff
Jason Frey
@Fryguy
Sep 02 2016 22:08
Perhaps the documentation should say that the method's purpose is to handle creating subselects
Keenan Brock
@kbrock
Sep 02 2016 22:08
It was able to get nick the 1000x speed increase, but it is tricky
Jason Frey
@Fryguy
Sep 02 2016 22:08
select_from_alias doesn't really tell me that
Nick LaMuro
@NickLaMuro
Sep 02 2016 22:08
hey, took me like a week staring at ar_virtual to realize that is basically what virtual_delegates are doing
Keenan Brock
@kbrock
Sep 02 2016 22:08
wow. only 1 week?
Jason Frey
@Fryguy
Sep 02 2016 22:08
Oh virtual delegates are really just subselects?
Keenan Brock
@kbrock
Sep 02 2016 22:08
you're wicked smart
+1
well
today, yes
Jason Frey
@Fryguy
Sep 02 2016 22:08
OH then that should be documented right up front :)
or just call them VirtualSubselects haha
Keenan Brock
@kbrock
Sep 02 2016 22:09
"tomorrow", they will leech into the main query (tomorrow = 9 months)
Nick LaMuro
@NickLaMuro
Sep 02 2016 22:09
@Fryguy What I meant by that is that is how they get translated to SQL
technically they also define a delegated method to do the same thing in ruby
Jason Frey
@Fryguy
Sep 02 2016 22:09
right
Keenan Brock
@kbrock
Sep 02 2016 22:09
but the intent is captured
so when we fix the code to be able to put in main query, we'll loose the subselect
it is a 10-20% hit
BUT
even with that hit, we're able to get 120s -> 15s
nick and I really want to get into main query, but baby steps
(sorry if I misspoke nick)
ooh, the column is an alias, virtual_alias. and this is the select clause for the virtual alias.
Nick LaMuro
@NickLaMuro
Sep 02 2016 22:16
To translate the "kbrockanese": basically virtual_delegates use Nested SQL queries for now, since they are easier, but they are slower than joins. But nested selects are WAY faster than pulling all the records down and filtering in ruby, so ¯\_(ツ)_/¯
Jason Frey
@Fryguy
Sep 02 2016 22:16
thanks got it ;)
Nick LaMuro
@NickLaMuro
Sep 02 2016 22:17

ooh, the column is an alias, virtual_alias. and this is the select clause for the virtual alias.

No idea about this bit... hieroglyphics maybe?

Keenan Brock
@kbrock
Sep 02 2016 22:17
lol
Jason Frey
@Fryguy
Sep 02 2016 22:17
we have emojis now for that
Keenan Brock
@kbrock
Sep 02 2016 22:17
need to upgrade my emoji library
Nick LaMuro
@NickLaMuro
Sep 02 2016 22:18
My next PR description will be entirely out of emojis, have fun!
Keenan Brock
@kbrock
Sep 02 2016 22:18
<3
I made a presentation 100% emojis
chess /fry didn't like it ;)
it is hard enough to understand my english
Jason Frey
@Fryguy
Sep 02 2016 22:19
sorry man, but that one was impossible haha
Keenan Brock
@kbrock
Sep 02 2016 22:19
haha
I'm sorry too
yea, naming for select_from_alias - maybe it aliased the table that we are selecting from.
it made sense back when I wrote it. (maybe - suspect but maybe)
Jason Frey
@Fryguy
Sep 02 2016 22:23

ooh, the column is an alias, virtual_alias. and this is the select clause for the virtual alias.

:scream:, :bar_chart: :arrow_forward: :copyright: , :ghost: :copyright: . :point_right: :ghost: :copyright:

(alias was hard :) I was looking for like icons for Copy/Paste but nada)
Keenan Brock
@kbrock
Sep 02 2016 22:24
lol
Nick LaMuro
@NickLaMuro
Sep 02 2016 22:31
hmm... maybe wingdings is a easier choice
Keenan Brock
@kbrock
Sep 02 2016 22:32
wish they had paste (glue) icon. that would be fun
@Fryguy ManageIQ/manageiq#10961 -- hash syntax did not work. broke a random test. reverted.
Nick LaMuro
@NickLaMuro
Sep 02 2016 22:33

wish they had paste (glue) icon. that would be fun

:poop: ? (sorry, couldn't resist)

Keenan Brock
@kbrock
Sep 02 2016 22:54
happy weekend