Skip to content

Commit

Permalink
Fix (HACK) N+1 with get_ems_metadata_tree hosts
Browse files Browse the repository at this point in the history
When building out hosts in get_ems_metadata_tree, there is a call to
`.v_total_vms` on the host records for each EmsCluster.  This ends up
causing an N+1 since the hosts from the tree data, and the `v_total_vms`
is a virtual_attribute that has to count all of the Vms for a given
host.

This change does a few things to avoid this (all of which are terrible
and none of which I am terribly proud of):

First, it adds an addition to `MiqPreloader`, which is a new method
`polymorphic_preload_for_child_classes`.  This allows requesting a
polymorphic relation to preload (top level only), and then you can
provide a collection of class names and scopes that can be applied for
the resulting classes that are return from the original preload for
further preloading on the child records.

This is then used by `miq_request_workflow.rb` and the Relationship
model and mixin to apply the scope and records to the original query and
records returned from it, preventing the N+1.
  • Loading branch information
NickLaMuro committed Apr 26, 2018
1 parent d986cb6 commit 7bc4621
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 4 deletions.
8 changes: 7 additions & 1 deletion app/models/miq_request_workflow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,13 @@ def get_ems_metadata_tree(src)
@ems_metadata_tree ||= begin
return if src[:ems].nil?
st = Time.zone.now
result = load_ar_obj(src[:ems]).fulltree_arranged(:except_type => "VmOrTemplate")
fulltree_opts = {
:except_type => "VmOrTemplate",
:class_specific_preloaders => {
EmsCluster => [:hosts, Host.select(Host.arel_table[Arel.star], :v_total_vms)]
}
}
result = load_ar_obj(src[:ems]).fulltree_arranged(fulltree_opts)
ems_metadata_tree_add_hosts_under_clusters!(result)
@ems_xml_nodes = {}
xml = MiqXml.newDoc(:xmlhash)
Expand Down
3 changes: 2 additions & 1 deletion app/models/mixins/relationship_mixin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,8 @@ def fulltree_ids_arranged(*args)

# Returns the records in the tree from the root arranged in a tree
def fulltree_arranged(*args)
Relationship.arranged_rels_to_resources(fulltree_rels_arranged(*args))
class_specific_preloaders = args.last.delete(:class_specific_preloaders) if args.last.is_a?(Hash)
Relationship.arranged_rels_to_resources(fulltree_rels_arranged(*args), true, class_specific_preloaders)
end

# Returns a list of all unique child types
Expand Down
7 changes: 5 additions & 2 deletions app/models/relationship.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,11 @@ def self.flatten_arranged_rels(relationships)
end
end

def self.arranged_rels_to_resources(relationships, initial = true)
MiqPreloader.preload(flatten_arranged_rels(relationships), :resource) if initial
def self.arranged_rels_to_resources(relationships, initial = true, class_specific_preloaders = nil)
if initial
record_set = flatten_arranged_rels(relationships)
MiqPreloader.polymorphic_preload_for_child_classes(record_set, :resource, class_specific_preloaders)
end

relationships.each_with_object({}) do |(rel, children), h|
h[rel.resource] = arranged_rels_to_resources(children, false)
Expand Down
64 changes: 64 additions & 0 deletions lib/miq_preloader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,68 @@ def self.preload_and_scope(records, association_name)
target_klass.where(join_key.key.to_sym => records.select(join_key.foreign_key.to_sym))
end
end

# Allows having a polymorphic preloader, but then having class specific
# preloaders fire for the loaded polymorphic classes.
#
# Example:
#
# irb> tree = ExtManagementSystem.all.fulltree_rels_arranged(:except_type => "VmOrTemplate")
# irb> records = Relationship.flatten_arranged_rels(tree)
# irb> hosts_scope = Host.select(Host.arel_table[Arel.star], :v_total_vms))
# irb> preloaders_per_class = { EmsCluster => [:hosts, hosts_scope] }
# irb> MiqPreloader.polymorphic_preload_for_child_classes(records, )
#
# Note: Class's .base_class are favored over their specific class
#
def self.polymorphic_preload_for_child_classes(records, associations, class_preloaders = {})
preloader = ActiveRecord::Associations::Preloader.new
preloader.extend(Module.new {
attr_accessor :class_specific_preloaders

# DIRTY HACK... but hey... at least I am just isolating it to this method
# right...
#
# Everyone else: "No Nick... just no..."
#
# Updated form from ar_virtual.rb, and merged with the code originally in
# ActiveRecord. If the code in ar_virtual.rb is changed, this should
# probably also be updated.
def preloaders_for_one(association, records, scope)
klass_map = records.compact.group_by(&:class)

loaders = klass_map.keys.group_by { |klass| klass.virtual_includes(association) }.flat_map do |virtuals, klasses|
subset = klasses.flat_map { |klass| klass_map[klass] }
preload(subset, virtuals)
end

records_with_association = klass_map.select { |k, rs| k.reflect_on_association(association) }.flat_map { |k, rs| rs }
if records_with_association.any?
# This injects the original code from preloaders_for_one from
# ActiveRecord so we can add our own `if` in the middle of it. The
# positive part of the `if` is the only portion of this that has
# changed, and the code copied is within the `loaders.concat`.
loaders.concat(grouped_records(association, records_with_association).flat_map do |reflection, klasses|
klasses.map do |rhs_klass, rs|
base_klass = rhs_klass.base_class if rhs_klass.respond_to?(:base_class)
loader = preloader_for(reflection, rs, rhs_klass).new(rhs_klass, rs, reflection, scope)
loader.run self
# Start of actual new code
if specific_klass = (class_specific_preloaders[base_klass] || class_specific_preloaders[base_klass])
[loader, MiqPreloader.preload(loader.preloaded_records, specific_klass[0], specific_klass[1])]
else
loader
end
# End of new code
end
end)
end

loaders
end
})
preloader.class_specific_preloaders = class_preloaders || {}

preloader.preload(records, associations, nil)
end
end

0 comments on commit 7bc4621

Please sign in to comment.