These are chat archives for ManageIQ/manageiq/performance

22nd
Aug 2016
Keenan Brock
@kbrock
Aug 22 2016 17:45
@Fryguy can you think of any reason we are comparing on ldap group description vs group.id? Maybe for region?
And why case insensitive if we are pulling them out of the same table?
# ownership_mixin.rb
  def owned_by_current_ldap_group
    ldap_group = User.current_user.try(:ldap_group)
    ldap_group && owning_ldap_group && (owning_ldap_group.downcase == ldap_group.downcase)
  end

# user:
  def ldap_group
    current_group.try(:description)
  end
  alias_method :miq_group_description, :ldap_group
Jason Frey
@Fryguy
Aug 22 2016 18:55
¯\_(ツ)_/¯ @abellotti ?
or @gtanzillo ?
Nick LaMuro
@NickLaMuro
Aug 22 2016 18:59
I think that is some @chessbyte code actually
but that might just be his commits extracting it into a mixin
Keenan Brock
@kbrock
Aug 22 2016 19:00
but agreed - best to fix this without changing that underlying logic
@NickLaMuro thanks so much for all your work/tests in ManageIQ/manageiq#10290
it is making a hypothesis I have so much easier to run
Nick LaMuro
@NickLaMuro
Aug 22 2016 19:39
Glad spending as much time on it had something good come of it
Keenan Brock
@kbrock
Aug 22 2016 19:42
lol. you did all the hard work
Keenan Brock
@kbrock
Aug 22 2016 19:50
@NickLaMuro what do you think about:
Vm.where(:name => LOWER("abc"), :id => GTEQ("5"))
Keenan Brock
@kbrock
Aug 22 2016 20:04
@NickLaMuro yea, so https://github.com/ManageIQ/manageiq/compare/master...kbrock:ownership_mixin_owned_by_ldap_sql_alt -- but that lack of inner join is getting me: no ldap group is failing for me
only the last commit shows what I was thinking of
My main goal is to remove the need for @named_scope and the logic to add it
Nick LaMuro
@NickLaMuro
Aug 22 2016 20:07

@NickLaMuro what do you think about:

Vm.where(:name => LOWER("abc"), :id => GTEQ("5"))

I like it, and don't have a problem it being included myself. Might be worth abstracting into a light gem though

Keenan Brock
@kbrock
Aug 22 2016 20:07
yea - with LOTS of tests
I ran by @Fryguy but he liked the above syntax better
Vm.where(:name => /^abc$/i)
Jason Frey
@Fryguy
Aug 22 2016 20:08
I was more concerned about co-opting regexes for things like lower
Keenan Brock
@kbrock
Aug 22 2016 20:08
+1
what you like is easier to write
Nick LaMuro
@NickLaMuro
Aug 22 2016 20:10
it would be interesting comparing it to squeel
for some of this, and see if there is performance gains/loses, and if devs hate/like one v.s. the other
Keenan Brock
@kbrock
Aug 22 2016 20:12
@Fryguy the regular expresion that co-opt: https://gist.github.com/kbrock/6c197a35d3243783fa476134d2b8b651
It was quite opressive
/collapse-all
@NickLaMuro in my little bubble, we can just use active record hashes, and we can implement search using the hashes in ruby.
so we don't need all this miq expression eval fu
but also in my bubble, we will go away from lower(col) and move towards citext or a database solution and drop all those LOWER functions
Nick LaMuro
@NickLaMuro
Aug 22 2016 20:16
huh... didn't notice how stagnant squeel was at this point... maybe forget I suggested that
Keenan Brock
@kbrock
Aug 22 2016 20:17
I used in a previous place. liked it alot. but the devs were very SQL savy
and we were doing geo queries / arrays and stuff. not very rails friendly queries. (things are different now)
Nick LaMuro
@NickLaMuro
Aug 22 2016 20:50
@kbrock I might have something that works with a inner select
give me a few to get tests passing and try something out
Keenan Brock
@kbrock
Aug 22 2016 20:54
+1
I found bug in evm_owner definition
seems odd that we are using vm.evm_owner_id -> users -> userid.downcase and comparing on both sides
@Fryguy do we use userid (external identifier) to match across regions instead of just using userid?
Keenan Brock
@kbrock
Aug 22 2016 21:07
finding bugs
but it is working
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:08
@kbrock this is the patch (left in the comments of other attempts):
diff --git a/app/models/mixins/ownership_mixin.rb b/app/models/mixins/ownership_mixin.rb
index 259c50e..9baf613 100644
--- a/app/models/mixins/ownership_mixin.rb
+++ b/app/models/mixins/ownership_mixin.rb
@@ -22,7 +22,19 @@ module OwnershipMixin
     virtual_attribute :owned_by_current_ldap_group, :boolean, :arel => (lambda do |t|
       ldap_group = User.current_user.try(:ldap_group).to_s.downcase

-      t.grouping(MiqGroup.arel_table[:description].lower.eq(ldap_group))
+      # t.grouping(MiqGroup.arel_table[:description].lower.eq(ldap_group))
+      to_ref = self.reflect_on_association(:miq_group)
+      src_model_id = arel_attribute(to_ref.association_foreign_key, t)
+      to_model_id = MiqGroup.arel_attribute(to_ref.active_record_primary_key)
+      # t.grouping(
+      #   # MiqGroup.select(t.grouping(MiqGroup.arel_table[:description].lower.eq(ldap_group)))
+      #   MiqGroup.select(%Q{(LOWER("miq_groups"."description") = #{ldap_group})})
+      #     .where(to_model_id.eq(src_model_id))
+      # )
+      # teh_scope = MiqGroup.select(%Q{(LOWER("miq_groups"."description") = "#{ldap_group}")})
+      teh_scope = MiqGroup.select(t.grouping(MiqGroup.arel_table[:description].lower.eq(ldap_group)))
+                    .where(to_model_id.eq(src_model_id)).to_sql
+      Arel.sql("(#{teh_scope})")
     end)
   end

diff --git a/spec/support/examples_group/shared_examples_for_ownership_mixin.rb b/spec/support/examples_group/shared_examples_for_ownership_mixin.rb
index 16f95f4..7c3efe0 100644
--- a/spec/support/examples_group/shared_examples_for_ownership_mixin.rb
+++ b/spec/support/examples_group/shared_examples_for_ownership_mixin.rb
@@ -18,7 +18,11 @@ shared_examples "miq ownership" do

       it "usable as arel" do
         group_name = user.current_group.description.downcase
-        sql        = %`(LOWER("miq_groups"."description") = '#{group_name}')`
+        sql        = <<-SQL.gsub(/^ */, '').split("\n").join(' ')
+                       (SELECT (LOWER("miq_groups"."description") = '#{group_name}')
+                        FROM "miq_groups"
+                        WHERE "miq_groups"."id" = "#{described_class.table_name}"."miq_group_id")
+                     SQL
         attribute  = described_class.arel_attribute(:owned_by_current_ldap_group)
         expect(stringify_arel(attribute)).to eq [sql]
       end
You basically can drop all of the @named_scope commits and just apply it directly to ManageIQ/manageiq@8339579
essentially, I just ripped off the :belongs_to arel from virtual_delegate
Keenan Brock
@kbrock
Aug 22 2016 21:09
there was a bug in there
it didn't properly read :foreign_key
can we just use virtual_delegate though?
this is very cool nick
I just want the world ;)
  included do
    belongs_to :evm_owner, :class_name => "User", :foreign_key => :evm_owner_id
    belongs_to :miq_group

    virtual_delegate :email, :name, :userid, :to => :evm_owner, :prefix => true, :allow_nil => true
    virtual_attribute :owned_by_current_user, :boolean, :uses => :evm_owner, :arel => (lambda do |t|
      #t.grouping(arel_attribute(:evm_owner_id)]).eq(User.current_user.try(:id)))
      t.grouping(
        Arel::Nodes::NamedFunction.new("LOWER", [arel_attribute(:evm_owner_userid)]).eq(User.current_userid.downcase)
      )
    end)
    virtual_column :owning_ldap_group,                    :type => :string,     :uses => :miq_group
    virtual_column :owned_by_current_ldap_group,          :type => :boolean,    :uses => :owning_ldap_group
  end
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:10
Not sure if virtual_delegate would coerce it to a boolean
Keenan Brock
@kbrock
Aug 22 2016 21:10
that is working for me
owned by current user == easier
would prefer the top one
but... oh well
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:11
Is that virtual_delegate stuff in master though? And would it get backported to Darga?
Keenan Brock
@kbrock
Aug 22 2016 21:12
VmOrTemplate.select(:name).order(Vm.arel_attribute(:evm_owner_userid)).limit(3)
SELECT  "vms"."name" FROM "vms" ORDER BY (SELECT "users"."userid" FROM "users" WHERE "users"."id" = "vms"."evm_owner_id") LIMIT $1  [["LIMIT", 3]]
well, THAT fix is in master
getting groups working - maybe not so easy
you have 2 PRs, one does user, the other does group AND user
I have an easy fix for user
will put together PR
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:13
right, but it requires ManageIQ/manageiq#10476
Keenan Brock
@kbrock
Aug 22 2016 21:13
the User one does not
it is darga compliant
maybe even c
the group one - right now does require that patch - you are correct
that is darga friendly as is
well, needs tests
aah
I see the rebase point - sorry / thanks
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:17
Yeah, that branch should do the trick, nevermind
I was reading the virtual_delegate line with :name => ...
Keenan Brock
@kbrock
Aug 22 2016 21:18
+1
yes, that will NOT work
maybe introducing a virtual attribute with the correct target name or something?
so we would need no name munging?
I'll think about this
seems this is very close
I really like what you did
also, delegate may not work for us, but we could properly define ownership_ldap_group - with arel, then this would be much easier
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:20
Hold off on the PR. I like your fix, but I had tests done in a way that we could continue off of them with what I did in the original: https://github.com/ManageIQ/manageiq/pull/10525/commits/53f086b9cbba856ac55d335b7c02ed1f622f5d52
Keenan Brock
@kbrock
Aug 22 2016 21:20
yea
I tried to chery-pick - failed
right now, virtual_delegates introduces inner queries
which both of us don't like
but the external api is very nice
I really like having virtual_delegate in the code
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:22
I am not really a fan of externally dependent joins though either
Keenan Brock
@kbrock
Aug 22 2016 21:22
so when you and I want to fix those stupid inner queries, we can tap into virtual delegate and fix them
agreed
the inner queries sure clean up the dependencies
as in, it removes @named_scope
you know what
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:23
Yeah, basically I can drop those last 3 commits in my first PR
Keenan Brock
@kbrock
Aug 22 2016 21:23
ownership mixin is pretty ugly
maybe we could not do it generically
hardcode the values
the arel would be much cleaner
and we could put a todo above it with the virtual delegate code
Oleg Barenboim
@chessbyte
Aug 22 2016 21:24
ARELize all the things
Keenan Brock
@kbrock
Aug 22 2016 21:24
lol
in my world, arel below the covers, dsl above
I like the amount of arel you see here:
    virtual_delegate :email, :name, :userid, :to => :evm_owner, :prefix => true, :allow_nil => true
Oleg Barenboim
@chessbyte
Aug 22 2016 21:25
waiting for each UI screen to snap back in under 2 seconds. ok under 5 seconds. ok under 10 seconds, but no more.
Keenan Brock
@kbrock
Aug 22 2016 21:25
:)
nick is all over that
he is here to make me look good too
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:26
Well, we got the 2 seconds part on these screens, now I am just trying to make @Fryguy and @kbrock happy
Keenan Brock
@kbrock
Aug 22 2016 21:26
6 months later...
Oleg Barenboim
@chessbyte
Aug 22 2016 21:26
I saw some of your PRs on Ancestry and C&U and saw 90k queries -- was just blown away by that magnitude
Keenan Brock
@kbrock
Aug 22 2016 21:26
we'll have it by f release ;)
nick is on fire
Oleg Barenboim
@chessbyte
Aug 22 2016 21:27
glad we got Nick on board -- will be exciting to see those screens snap back
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:27
:ship: :fire:
Oleg Barenboim
@chessbyte
Aug 22 2016 21:27
even if by Fischer release
Keenan Brock
@kbrock
Aug 22 2016 21:27
I'm just razzing
Oleg Barenboim
@chessbyte
Aug 22 2016 21:27
oops F release
Keenan Brock
@kbrock
Aug 22 2016 21:27
lol
not sure how we can have any other f release
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:28
that name has been set in stone, hasn't it
Keenan Brock
@kbrock
Aug 22 2016 21:28
:+1:
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:28
:fishing_pole_and_fish:
Oleg Barenboim
@chessbyte
Aug 22 2016 21:28
his anti-semitic rants may sway people to vote for someone else
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:29
that is fair
also, TIL
Keenan Brock
@kbrock
Aug 22 2016 21:29
I don't get out much and hadn't heard about that
Oleg Barenboim
@chessbyte
Aug 22 2016 21:29
not to mention him being a fugitive of the USA because of tax evasion and publicly spitting and tearing up federal documents
Keenan Brock
@kbrock
Aug 22 2016 21:29
ok
a lot of very not cool stuff
I didn't know about any of this
there was a cool movie about him though...
Oleg Barenboim
@chessbyte
Aug 22 2016 21:29
he was just plain crazy
Pawn Sacrifice
Keenan Brock
@kbrock
Aug 22 2016 21:30
those chess people typically are ;)
Oleg Barenboim
@chessbyte
Aug 22 2016 21:30
with Toby McGuire and Liev Schreiber
Keenan Brock
@kbrock
Aug 22 2016 21:30
show me the money guy?
I know very litle about the chess world. Glad we cold name a branch eww though
and casablanca
Oleg Barenboim
@chessbyte
Aug 22 2016 21:31
better than yuck
Keenan Brock
@kbrock
Aug 22 2016 21:31
lol
ok, capablanka - that I will always remember
Oleg Barenboim
@chessbyte
Aug 22 2016 21:31
capablanca was a suave gentleman from Cuba
a ladies man
Keenan Brock
@kbrock
Aug 22 2016 21:32
no way. so it is very similar to spanish white house
since he WAS from cuba
very fun
ok
nick
how do we get these fixes for user in?
Keenan Brock
@kbrock
Aug 22 2016 21:32
and into darga
whoa - rico suave
Oleg Barenboim
@chessbyte
Aug 22 2016 21:34
does this photo of Capa remind you of anything?
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:35
hah!
Keenan Brock
@kbrock
Aug 22 2016 21:35
30!
oleg - you have to step up your game
well, we all have to step up OUR game. YOU brought yours and kicked it
(edited for child friendly audiences)
Oleg Barenboim
@chessbyte
Aug 22 2016 21:36
I have many years of practice
but still suck, IMO
Keenan Brock
@kbrock
Aug 22 2016 21:37
for some definition of suck
so C played E and B
ooh, and Fisher and others
was Flear any good? he is an F
ugh
nick
how are we going to merge the User fixes?
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:39
@kbrock to answer your question, I think this is how we should go about getting in these changes:
  • I fix ManageIQ/manageiq#10290 by applying the patch shared above (or some better variant of it)
    • get new metrics
    • confirm it works
    • etc.
  • You then can either Branch off of that or wait until it is merged to add your changes
Keenan Brock
@kbrock
Aug 22 2016 21:40
ok
so you just said you will change 10290 so it is NOT using @named_scope?
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:40
yeah
Keenan Brock
@kbrock
Aug 22 2016 21:40
and we merge
and we backport
then I can possible switch so basically it adds virtual column/delegate for the group stuff
that sounds great
what about user?
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:41
basically, the virtual_attribute for owned_by_current_ldap_user will change to this:
     virtual_attribute :owned_by_current_ldap_group, :boolean, :arel => (lambda do |t|
       ldap_group = User.current_user.try(:ldap_group).to_s.downcase

      t.grouping(MiqGroup.arel_table[:description].lower.eq(ldap_group))
      to_ref = self.reflect_on_association(:miq_group)
      src_model_id = arel_attribute(to_ref.association_foreign_key, t)
      to_model_id = MiqGroup.arel_attribute(to_ref.active_record_primary_key)
      teh_scope = MiqGroup.select(t.grouping(MiqGroup.arel_table[:description].lower.eq(ldap_group)))
                    .where(to_model_id.eq(src_model_id)).to_sql
      Arel.sql("(#{teh_scope})")
     end)
   end
(sorry bout the formatting, but WONTFIX)
Keenan Brock
@kbrock
Aug 22 2016 21:42
lol
+1
ok
that OR
hardcode src_model_id, to_model_id
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:42
I might make those methods
or class attributes
they shouldn't be looked up every time
Keenan Brock
@kbrock
Aug 22 2016 21:43
just hardcode to "id"
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:43
sure
Keenan Brock
@kbrock
Aug 22 2016 21:43
never mind - that sounds great
what ever you want to do
in the weeds
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:43
above is just a POC, it looks like crap
Keenan Brock
@kbrock
Aug 22 2016 21:44
prefer you just use MiqGroup.arel_table[:description] like stuff
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:44
I was mostly testing that I could:
  • Do this with a inner query
  • Could cast to a boolean in the SELECT of the inner query

prefer you just use MiqGroup.arel_table[:description] like stuff

Honestly, I was just copying what I had, so I don't actually have a preference

Keenan Brock
@kbrock
Aug 22 2016 21:45
once delegates PR is in, and it will probably be darga/no, then I will want to convert over to delegates
+1
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:45
can I ask what the difference is between the arel_table and the arel_attribute?
Keenan Brock
@kbrock
Aug 22 2016 21:46
so just put in either for quickness (as is) or for easy to read (arel_table...)
just use arel_table
arel_attribtue ==>
the lambda you are adding ties into arel attribute
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:47
oh, I didn't see this is added in ar_virtual
I just thought this shipped with activerecord
Keenan Brock
@kbrock
Aug 22 2016 21:47
arel_table just does "#{tablename}.#{column}"
ar_virtual hacks it
but it is in activerecord
Nick LaMuro
@NickLaMuro
Aug 22 2016 21:47
okay, then continue (just noticed the super in that method again...)
Keenan Brock
@kbrock
Aug 22 2016 21:47
it allows us to modify it before just doing the dumb version
ar_virtual asks if WE stored special arel
and enhances the arel_attribute
so it calls things like this arel (the lambda you are writing) and generates better arel - instead of the naieve version from active record
active record has very basic aliases - alias_attribute
but that is about it
we took to the next level and let you specify better sql than just swap this name for this name.
we provide a lambda and it will generate based upon who knows what
maybe even look at the current query and do funky stuff
currently, we only construct case statements / lower statements / inner selects and stuff like that
dinner - be back in a bit
Nick LaMuro
@NickLaMuro
Aug 22 2016 23:17
@kbrock here is the new PR (decided to make a new one so we could compare both approaches): ManageIQ/manageiq#10685
we eat a bit on performance gains by not doing a JOIN, but probably a better overall to avoid code clutter
Keenan Brock
@kbrock
Aug 22 2016 23:51
thanks
yes
and we ahve a plan to remove this performance hit
yay