[PATCH conductor] BZ #794759 - Pool Family page no longer runs thousands of SQL queries

Tomas Sedovic tsedovic at redhat.com
Mon Feb 20 15:38:08 UTC 2012


On 02/17/2012 05:47 PM, Matt Wagner wrote:
> This beats PoolFamily#statistics and Pool#statistics into shape, and
> also avoids invoking the method over and over again in the views.
>
> Resolves https://bugzilla.redhat.com/show_bug.cgi?id=794759
> ---
>   src/app/models/instance.rb                   |    2 +
>   src/app/models/pool.rb                       |    3 +-
>   src/app/models/pool_family.rb                |    9 ++++---
>   src/app/views/pool_families/_list.html.haml  |   28 +++++++++++++------------
>   src/app/views/pool_families/_pools.html.haml |   15 +++++++------
>   src/app/views/pools/_list.html.haml          |    9 ++++---
>   6 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
> index a7a82b5..c2a1d04 100644
> --- a/src/app/models/instance.rb
> +++ b/src/app/models/instance.rb
> @@ -119,6 +119,8 @@ class Instance<  ActiveRecord::Base
>     scope :pending,   :conditions =>  { :state =>  [STATE_NEW, STATE_PENDING] }
>     # FIXME: "failed" is misleading too...
>     scope :failed,    :conditions =>  { :state =>  [STATE_CREATE_FAILED, STATE_ERROR, STATE_VANISHED] }
> +  scope :stopped,   :conditions =>  {:state =>  STATE_STOPPED}
> +  scope :not_stopped, :conditions =>  "state<>  'stopped'"
>     scope :stopable,    :conditions =>  { :state =>  [STATE_NEW, STATE_PENDING, STATE_RUNNING] }
>     scope :ascending_by_name, :order =>  'instances.name ASC'
>
> diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
> index 8529a9d..b1fc41e 100644
> --- a/src/app/models/pool.rb
> +++ b/src/app/models/pool.rb
> @@ -92,8 +92,7 @@ class Pool<  ActiveRecord::Base
>       statistics = {
>         :cloud_providers =>  instances.collect{|i| i.provider_account}.uniq.count,
>         :deployments =>  deployments.count,
> -      :total_instances =>  (instances.deployed.count +
> -                           instances.pending.count + instances.failed.count),
> +      :total_instances =>  instances.not_stopped.count,
>         :instances_deployed =>  instances.deployed.count,
>         :instances_pending =>  instances.pending.count,
>         :instances_failed =>  failed,
> diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb
> index c75de3d..f1d0746 100644
> --- a/src/app/models/pool_family.rb
> +++ b/src/app/models/pool_family.rb
> @@ -74,11 +74,12 @@ class PoolFamily<  ActiveRecord::Base
>       max = quota.maximum_running_instances
>       total = quota.running_instances
>       avail = max - total unless max.nil?
> +    my_pools = pools

could you change this to `cached_pools` or add a little comment there? 
Just so that someone doesn't change it back to the `pools` call in 6 months.

>       statistics = {
> -      :deployments =>  pools.collect{|p| p.statistics[:deployments]}.sum,
> -      :total_instances =>  pools.collect{|p| p.statistics[:total_instances]}.sum,
> -      :instances_pending =>  pools.collect{|p| p.statistics[:instances_pending]}.sum,
> -      :instances_failed =>  pools.collect{|p| p.statistics[:instances_failed_count]}.sum,
> +      :deployments =>  my_pools.collect{|p| p.deployments.count}.sum,
> +      :total_instances =>  my_pools.collect{|p| p.instances.not_stopped.count}.sum,
> +      :instances_pending =>  my_pools.collect{|p| p.instances.pending.count}.sum,
> +      :instances_failed =>  my_pools.collect{|p| p.instances.failed.count}.sum,
>         :used_quota =>  total,
>         :quota_percent =>  number_to_percentage(quota.percentage_used,
>                                                :precision =>  0),
> diff --git a/src/app/views/pool_families/_list.html.haml b/src/app/views/pool_families/_list.html.haml
> index 40ca206..cde9905 100644
> --- a/src/app/views/pool_families/_list.html.haml
> +++ b/src/app/views/pool_families/_list.html.haml
> @@ -7,6 +7,7 @@
>
>   .content
>     - @pool_families.each do |pool_family|
> +    - @family_stats = pool_family.statistics

Any reason to not use a local variable here ^ instead?

>       %table.pool_families
>         %caption
>           .caption_content
> @@ -31,27 +32,28 @@
>             %th= t("pool_families.index.catalog")
>             %th
>           - pool_family.pools.each do |pool|
> +          - @pool_stats = pool.statistics

ditto ^

>             %tr
>               %td= link_to pool.name, pool
>               %td= pool.deployments.count
> -            %td= pool.statistics[:total_instances]
> -            %td= pool.statistics[:instances_pending]
> -            %td= pool.statistics[:instances_failed_count]
> -            %td= pool.statistics[:quota_percent]
> -            %td= pool.statistics[:used_quota]
> -            %td= pool.statistics[:available_quota].nil? ? raw('&infin;') : pool.statistics[:available_quota]
> +            %td= @pool_stats[:total_instances]
> +            %td= @pool_stats[:instances_pending]
> +            %td= @pool_stats[:instances_failed_count]
> +            %td= @pool_stats[:quota_percent]
> +            %td= @pool_stats[:used_quota]
> +            %td= @pool_stats[:available_quota].nil? ? raw('&infin;') : @pool_stats[:available_quota]
>               %td= pool.enabled? ? t("pool_families.index.answer_yes") : t("pool_families.index.answer_no")
>               %td= link_to pool.catalogs.first.name, catalog_path(pool.catalogs.first) if pool.catalogs.any?
>               %td= link_to t(:edit), edit_pool_path(pool), :class =>  'pool_family_button'
>           %tr
>             %td= t("pool_families.index.total_statistics")
> -          %td= pool_family.statistics[:deployments]
> -          %td= pool_family.statistics[:total_instances]
> -          %td= pool_family.statistics[:instances_pending]
> -          %td= pool_family.statistics[:instances_failed]
> -          %td= pool_family.statistics[:quota_percent]
> -          %td= pool_family.statistics[:used_quota]
> -          %td= pool_family.statistics[:available_quota].nil? ? raw('&infin;') : pool_family.statistics[:available_quota]
> +          %td= @family_stats[:deployments]
> +          %td= @family_stats[:total_instances]
> +          %td= @family_stats[:instances_pending]
> +          %td= @family_stats[:instances_failed]
> +          %td= @family_stats[:quota_percent]
> +          %td= @family_stats[:used_quota]
> +          %td= @family_stats[:available_quota].nil? ? raw('&infin;') : @family_stats[:available_quota]
>             %td
>             %td
>             %td
> diff --git a/src/app/views/pool_families/_pools.html.haml b/src/app/views/pool_families/_pools.html.haml
> index 68de8c6..e9b03d0 100644
> --- a/src/app/views/pool_families/_pools.html.haml
> +++ b/src/app/views/pool_families/_pools.html.haml
> @@ -1,12 +1,13 @@
>   = filter_table(@pools_header, @pool_family.pools) do |pool|
> +  - pool_stats = pool.statistics
>     %tr{:class =>  cycle('nostripe','stripe')}
>       %td= link_to pool.name, pool
>       %td{:class =>  'center'}= pool.deployments.count
> -    %td{:class =>  'center'}= pool.statistics[:total_instances]
> -    %td{:class =>  'center'}= pool.statistics[:instances_pending]
> -    %td{:class =>  'center'}= pool.statistics[:instances_failed_count]
> -    %td{:class =>  'center'}= pool.statistics[:quota_percent]
> -    %td{:class =>  'center'}= pool.statistics[:used_quota]
> -    %td{:class =>  'center'}= pool.statistics[:available_quota].nil? ? raw('&infin;') : pool.statistics[:available_quota]
> +    %td{:class =>  'center'}= pool_stats[:total_instances]
> +    %td{:class =>  'center'}= pool_stats[:instances_pending]
> +    %td{:class =>  'center'}= pool_stats[:instances_failed_count]
> +    %td{:class =>  'center'}= pool_stats[:quota_percent]
> +    %td{:class =>  'center'}= pool_stats[:used_quota]
> +    %td{:class =>  'center'}= pool_stats[:available_quota].nil? ? raw('&infin;') : pool_stats[:available_quota]
>       %td= link_to pool.catalogs.first.name, catalog_path(pool.catalogs.first) if pool.catalogs.any?
> -    %td= link_to t(:edit), edit_pool_path(pool), :class =>  'rounded-link'
> \ No newline at end of file
> +    %td= link_to t(:edit), edit_pool_path(pool), :class =>  'rounded-link'
> diff --git a/src/app/views/pools/_list.html.haml b/src/app/views/pools/_list.html.haml
> index 10bd342..5dc5e5d 100644
> --- a/src/app/views/pools/_list.html.haml
> +++ b/src/app/views/pools/_list.html.haml
> @@ -24,16 +24,17 @@
>       });
>
>   = filter_table(@header, @pools) do |pool|
> +  - pool_stats = pool.statistics
>     %tr{:class =>  cycle('nostripe','stripe')}
>       %td{:class =>  'checkbox'}
>         %input{:name =>  "pools_selected[]", :type =>  "checkbox", :value =>  pool.id, :id =>  "pool_checkbox_#{pool.id}" }
>       %td{:class =>  'alert'}= (@statistics[:instances_failed]&  pool.instances).present? ? image_tag("sb_icon_instance_failure.png") : ""
>       %td= link_to(pool.name, pool_path(pool, :view =>  :filter))
>       %td{:class =>  'center'}= pool.deployments.count
> -    %td{:class =>  'center'}= pool.statistics[:total_instances]
> -    %td{:class =>  'center'}= pool.statistics[:instances_pending]
> -    %td{:class =>  'center'}= pool.statistics[:instances_failed_count]
> -    %td{:class =>  'center'}= pool.statistics[:quota_percent]
> +    %td{:class =>  'center'}= pool_stats[:total_instances]
> +    %td{:class =>  'center'}= pool_stats[:instances_pending]
> +    %td{:class =>  'center'}= pool_stats[:instances_failed_count]
> +    %td{:class =>  'center'}= pool_stats[:quota_percent]
>       - if check_privilege(Privilege::VIEW, pool.pool_family)
>         %td= link_to(pool.pool_family.name, pool.pool_family)
>       - else

The speedup on my side was much less pronounced (~150ms) but we should 
not be calling the database like that, so ACK.

There's a couple of notes inline, feel free to disregard them if you 
disagree.



More information about the aeolus-devel mailing list