These are chat archives for ManageIQ/manageiq/performance

2nd
Feb 2017
Beni Cherniavsky-Paskin
@cben
Feb 02 2017 14:15
I have the anecdotal feeling that in development static asset serving is annoyingly slow.
I tried htop, perf top, and https://github.com/jvns/ruby-stacktrace while reloading 2 tabs (to fetch lots of assets), and noticed couple suspicious things:
  1. puma is CPU-bound? main process cpu ~97%. there are 5 or 6 active threads, which seem to add up towards 100%. (so no parallelism => pure-ruby-bound?)
  2. ruby-stacktrace shows ~30% in active_support/file_update_checker.rb. Unclear what the absolute numbers mean with threads (jvns/ruby-stacktrace#21), but anyway.
    Does the Rails reloader recheck my whole filesystem for every single asset ?!
    Yes! strace -y -f -p $(pgrep -f puma), then hit one asset eg. http://localhost:3000/assets/100/policy_profile-6989dc615a221c3729bb6b946d3e2400a879b4bba0e6e76712271bb0908f983d.png => massive stat storm!
So, :cherries: — is there easy way to disable or rate-limit file_update_checker for assets/requests? Or maybe make it use inotify?
Nick LaMuro
@NickLaMuro
Feb 02 2017 14:16
Short answer is yes, but that is by design
Joe Rafaniello
@jrafanie
Feb 02 2017 14:17
Interesting @cben, that's what I'd expect, it's looking for changes
one workaround in dev mode is to bump your database.yml pool size to 20 or so since all dev mode asset request goes through rails/puma
Nick LaMuro
@NickLaMuro
Feb 02 2017 14:18
Or, compiling the assets and running in RAILS_ENV=production mode
Joe Rafaniello
@jrafanie
Feb 02 2017 14:18
yeah
@NickLaMuro didn't we find that Matthew or someone fixed the rack middleware needing a db connection thing recently?
(even on asset requests that is)
I don't remember
Nick LaMuro
@NickLaMuro
Feb 02 2017 14:22

@jrafanie Thought it was related to this: ManageIQ/manageiq#12595

More specifically, I think you identified it as this commit: rails/rails@2b5d139

@cben To expand on my answer a bit, basically, in development mode, we don't compile assets, which means what is usually a compressed single .js and .css file for prod is dozens of individual files/libs split up per request that need to be served by puma
in Prod, those are actually just served by apache, since they are static
the "by design" part is this allows frontend devs to make changes to frontend assets without restarting the server/recompiling assets
but that means there is directory scanning done on every request, and things take much longer, especially since there is so many more files that need to be downloaded
Nick LaMuro
@NickLaMuro
Feb 02 2017 14:27
you probably should be able to configure Rails to server compiled assets in dev mode, so you can take advantage of the reloading of your ruby classes, but ignore the fact that you probably aren't changing anything in the asset portion of things (I am guessing)
personally, I always run in production with RAILS_ENV=production bin/rake assets:precompile since I am usually benchmarking the application anyway, so all of the caching mechanisms in play to get more realistic numbers
I find that restarting the server and the slowness of the initial request usually doesn't slow me down to much when working on performance stuff, but your milage may vary when working on a feature
Beni Cherniavsky-Paskin
@cben
Feb 02 2017 14:33
I'm cool with reloader & not compiling (tried it once, didn't realize I should clean them after, took a byebug trip through sprocket to figure my silliness, avoiding it ever since ;-)).
But (A) if a 100 requests arrive in same second, it's silly to scan FS 100 times. Let's say not re-checking if checked within last 1second might be a win but still allow reloading.
(B) it scans all ruby files. I'd like a mode where it rechecks only the relevant asset file(s) and assumes ruby code didn't change in ways that affect asset serving. [Again, could re-check if >N seconds passed to avoid endless debugging when you break that assumption but don't realize it was there.]
I suppose I should disable reloader to see if total page load improves at all.
Beni Cherniavsky-Paskin
@cben
Feb 02 2017 14:53
Wow. signed out login page seems like good test for many assets little backend work. Finish: 17s, 20s, 21s (according to bottom of chrome's Network tab).
Added return false at top of FileUpdateChecker#updated?. Finish: 10s, 6s, 7s! :cherries:
The syscalls it does now for one image are much saner: https://gist.github.com/cben/ca7a0d75734216816b39afdec5865224
/collapse (have to open link to see rest anyway)
Beni Cherniavsky-Paskin
@cben
Feb 02 2017 14:59
more or less directory listing along the path to the file + sprockets cache file (+ a dozen stat("/etc/localtime") ?)
Beni Cherniavsky-Paskin
@cben
Feb 02 2017 15:05
also interesting that ruby-stacktrace pointed to watched and max_mtime as 1st and 2nd most heavy. [https://github.com/rails/rails/blob/aa647b46/activesupport/lib/active_support/file_update_checker.rb#L100-L139]
watched just globs all the files, potentially redundant work with stat()ing them later?
Is it possible there is no existing gem to use inotify to trigger reloading?
Keenan Brock
@kbrock
Feb 02 2017 15:10
@NickLaMuro I'm noticing a common trend
Nick LaMuro
@NickLaMuro
Feb 02 2017 15:11
There is the guard gem that might have Rails assets plugin you could use, but I am thinking that it isn't baked into Rails because of cross platform issues (for the record, inotifty is something I only understand from a high level)
Keenan Brock
@kbrock
Feb 02 2017 15:11
MiqPolicySet#active?
It checks a value for all of the acts_as_miq_set/ relationship mixin / alias_with_relationship_type
it ends up with 2 queries (-> N+1 for page), but in the end of the day, it could have been a subquery / easy check
  EVENT_GROUPS_EXCLUDED = ["evm_operations", "ems_operations"]
  def self.all_policy_events
    # before
    all_events.select do |e|
      first_member = e.memberof.first.name
      # aka: first_member = e.relationships.first.root.resource.name 
      first_member.present? && !EVENT_GROUPS_EXCLUDED.include?(first_member)
    end
    # after
    MiqEventDefinition.all_events.where(:id =>
      Relationship.where(:resource_type => "MiqEventDefinition", :relationship => "membership") # relationships
                  .joins("JOIN relationships rel2 ON rel2.id = cast(split_part(relationships.ancestry, '/', 1) as bigint) AND rel2.resource_type = 'MiqEventDefinitionSet'") # root
                  .joins("JOIN miq_sets ON miq_sets.id = rel2.resource_id") # resource
                  .where(%{NOT ("relationships"."ancestry" = '' OR "relationships"."ancestry" IS NULL)}) #first_member.present?
                  .where.not(:miq_sets => {:name => EVENT_GROUPS_EXCLUDED}) #!EVENT_GROUPS_EXCLUDED.include?
                  .select(:resource_id)
    )
  end
the ruby is much prettier
but interestingly, the same exact pattern is in MiqPolicySet#active?
  def active? ##
    members.any?(&:active)
  end
seeing this same pattern twice (so far) on 1 page has me wondering
Keenan Brock
@kbrock
Feb 02 2017 15:16
but the after is very ugly
Nick LaMuro
@NickLaMuro
Feb 02 2017 15:46
@chrisarcand and myself had a mutual co-worker that coined a phrase in our office that "performance is sexy"... I think he was full of it
or rather, "performance is sexy... to the end user"
Nick LaMuro
@NickLaMuro
Feb 02 2017 15:51
basically what I am trying to say is very rarely do you have something with a large scope, that is performant, and pretty to look at (code base is what I am referring to)
Keenan Brock
@kbrock
Feb 02 2017 15:52
yea, but the N+1 version (before) is so much prettier and evident than the 1 version (after)
Nick LaMuro
@NickLaMuro
Feb 02 2017 15:52
sure, sidekiq is fast, but it is also a super small lib and has a mostly really nice codebase to look at, so it doesn't need to deal with the filtering that is necessary to do something like you just did
what you are creating is a readable "public interface" for other devs to use, which really should be the part that is named well and "pretty"
and hopefully the underlying method to get there is well tested and documented, and if we are lucky, doesn't need to change much
(making the "pretty" part less valuable)
Keenan Brock
@kbrock
Feb 02 2017 15:54
heh. test?
good idea, I'll start there :(
Nick LaMuro
@NickLaMuro
Feb 02 2017 15:58
keep in mind, while you didn't make the method itself "pretty", you made the interface pretty, because now it is an active record relation, and can easily be paired with pagination, and other scopes
filtering further with the array that is returned would require more select {|e| ... }'s... so you would be passing around the "ugly"
@kbrock one thing that you could argue is that you are breaking the "law of demeter" by including logic from that should exist in the Relationship class
Keenan Brock
@kbrock
Feb 02 2017 16:13
trying hard to get that concept into relationship
how you say, I want to filter by an attribute in the root
or filter by ensuring that a child has an attribute
if only we could change the way ancestry was written ;)
Keenan Brock
@kbrock
Feb 02 2017 17:20

@NickLaMuro do you think we could get this merged association.flat_map that would basically be an in?

E.g.

ExtManagementSystem.all.flat_map(:host)
# ==> behind the scenes / it would remain a relation
Host.where(:id => ExtManagementSystem.all.select(:host_id))
Keenan Brock
@kbrock
Feb 02 2017 22:13

@NickLaMuro
working:

select id, cast(unnest(regexp_split_to_array(relationships.ancestry, '/')) as bigint) as parent_id
from relationships
where (NOT (("relationships"."ancestry" = '' OR "relationships"."ancestry" IS NULL)))
limit 5;
       id       |   parent_id    
----------------+----------------
 10000000000002 | 10000000000001
 10000000000003 | 10000000000001
 10000000000004 | 10000000000001
 10000000000005 | 10000000000001
 10000000000006 | 10000000000001

not working (joining to the parent relationship):

select relationships.id
from relationships
join relationships rel2 on rel2.id = (cast(unnest(regexp_split_to_array(relationships.ancestry, '/')) as bigint))
where (NOT (("relationships"."ancestry" = '' OR "relationships"."ancestry" IS NULL)))
limit 5;
ERROR:  argument of JOIN/ON must not return a set
I want all the parent resources.
I was able to do it for "root" - since there is only 1 of them.
my final query is much longer :) - but have been able to get it into ruby. which is good
tangent: (working)
  def self.all_policy_events
    MiqEventDefinition.all_events.where(:id =>
      Relationship.where(:resource_type => name, :relationship => default_relationship_type).select(:resource_id)
                  .with_root(MiqEventDefinitionSet)
                  .where.not(MiqEventDefinitionSet.table_name.to_sym => {:name => EVENT_GROUPS_EXCLUDED})
    )
  end
I'm trying to get with_parents(MiqPolicy) working :)
easy to get this stuff working in sql, but in active record friendly terms, sometimes not so easy
Keenan Brock
@kbrock
Feb 02 2017 23:09
hmm. maybe not so easy to do this in sql