Load Lans effeciently in MiqProvisionVirtWorkflow #17381
Conversation
NickLaMuro
force-pushed
the
faster_lan_lookup_in_miq_provision_virt_workflow
branch
2 times, most recently
from
6257d58
to
3dcee40
Compare
Instead of preloading Lan records, splitting them up based on if they are shareable and instantiating entire ActiveRecord to only use the name column in a hash, use a specific query to gather and filter those values and generate a hash from that.
|
Checked commit NickLaMuro@3dcee40 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
| @lan11 = FactoryGirl.create(:lan, :name => "lan_A", :switch_id => s11.id) | ||
| @lan12 = FactoryGirl.create(:lan, :name => "lan_B", :switch_id => s12.id) | ||
| @lan13 = FactoryGirl.create(:lan, :name => "lan_C", :switch_id => s13.id) | ||
| @lan14 = FactoryGirl.create(:lan, :name => "lan_D", :switch_id => s14.id) |
There was a problem hiding this comment.
@NickLaMuro - Were the @lan14 and @other_vm additions to shore up the spec test cases or for another reason?
There was a problem hiding this comment.
@syncrou Took me a bit to remember why I did it myself!
I was actually to make sure that this test:
expect { workflow.load_hosts_vlans(hosts, {}) }.not_to exceed_query_limit(1)Was actually being exercised properly when multiple hosts were present. If I left the data as it was, I would have technically passed that spec without actually avoiding an N+1 because we were only testing against 1 host (note, the previous code did a hosts.each. Just wanted to make sure there wasn't a hidden N+1 in there by testing against multiple hosts.
| @@ -230,8 +230,6 @@ def available_vlans_and_hosts(options = {}) | |||
| hosts = get_selected_hosts(src) | |||
| unless @vlan_options[:vlans] == false | |||
| rails_logger('allowed_vlans', 0) | |||
| # TODO: Use Active Record to preload this data? | |||
| MiqPreloader.preload(hosts, :lans => :switches) | |||
There was a problem hiding this comment.
@NickLaMuro - Is pulling this out a dependency on #17354?
There was a problem hiding this comment.
Nope, don't think so, though maybe I am not understanding the question.
That was how hosts, lans, and switches were being loaded previously, and how "zero SQL queries" were being made in the test previously. This was just using the MiqPreloader.preload code that already existed, and I was just removing it since it was no longer needed.
Did that answer the question?
There was a problem hiding this comment.
Basically, in the previous versions of the code, this prevented N+1's in the load_hosts_vlans method. This is no longer needed, and avoids a lot of unneeded object allocations.
|
@miq-bot add_label fine/yes, gaprindashvili/yes |
Instead of preloading
Lanrecords, splitting them up based on if they are shareable and instantiating entireActiveRecordinstances to only use the name column in a hash, use a specific query to gather and filter those values and generate a hash from that.IMPORTANT NOTE: The provider PRs linked below should be merged first:
UPDATE: These PRs have been merged, so this is good to be merged now.
As the performance impact from removing the
MiqPreloader.preloadin this PR will then not be an issue.Benchmarks
The benchmark script used here basically runs the following:
And is targeting a fairly large EMS, with about 600 hosts.
Note: The benchmarks for this change do NOT include the changes from other PRs (#17354 for example). Benchmarks of all changes can be found here.
Links