Prevents N+1 SQL queries in miq_request_workflow.rb #17354

NickLaMuro · 2018-04-26T20:56:31Z

This has 3 different fixes for 3 different N+1's:

Handles .writable_storages with MiqPreloader properly
Collects all host records at once in .get_selected_hosts
~~Does a HACK to MiqPreloader to handle polymorphic relationships, and allows the .get_ems_metadata_tree to properly preload hosts.~~

~~The hack should be isolated to just this methods that need it, and should work just like the normal MiqPreloader.preload when there isn't a hash passed in, but it is admittedly pretty gross...~~

Update: The "HACK" has been rebased out and will be done in a separate branch.

Benchmarks

UPDATE: These are outdated metrics. See this comment for more accurate metrics based on the current changes.

The benchmark script used here basically runs the following:

ManageIQ::Providers::Vmware::InfraManager::ProvisionWorkflow.new.init_from_dialog

And is targeting a fairly large EMS, with about 600 hosts.

	ms	queries	rows
before	15023	1961	48395
after	11722	59	48395

Before                                                     After
------                                                     -----

Total allocated: 2069091751 bytes (23226536 objects)   |   Total allocated: 1975603612 bytes (21900482 objects)
                                                       |   
allocated objects by gem                               |   allocated objects by gem
-----------------------------------                    |   -----------------------------------
   9665227  pending                                    |      9665227  pending
   5561668  manageiq/app  <<<<<<<<<<                   |      5557939  manageiq/app  <<<<<<<<<<
   3513258  manageiq/lib  <<<<<<<<<<                   |      3508974  manageiq/lib  <<<<<<<<<<
   2512007  activerecord-5.0.7  <<<<<<<<<<             |      1576834  activerecord-5.0.7  <<<<<<<<<<
    861270  activesupport-5.0.7  <<<<<<<<<<            |       808731  activesupport-5.0.7  <<<<<<<<<<
    418737  ancestry-2.2.2                             |       418737  ancestry-2.2.2
    278793  activemodel-5.0.7  <<<<<<<<<<              |       274449  activemodel-5.0.7  <<<<<<<<<<
    178419  ruby-2.3.3/lib  <<<<<<<<<<                 |        52875  manageiq-providers-vmware-0be2f13a0dc9
    165577  arel-7.1.4                                 |        14424  fast_gettext-1.2.0
     52875  manageiq-providers-vmware-0be2f13a0dc9     |        13366  arel-7.1.4  <<<<<<<<<<
     14424  fast_gettext-1.2.0                         |         8453  ruby-2.3.3/lib  <<<<<<<<<<
      4115  other                                      |          309  other
        75  default_value_for-3.0.5                    |           75  default_value_for-3.0.5
        68  concurrent-ruby-1.0.5                      |           66  concurrent-ruby-1.0.5
        16  memoist-0.15.0                             |           16  memoist-0.15.0
         4  rubygems                                   |            4  rubygems
         3  manageiq-providers-lenovo-470f5d68672b     |            3  manageiq-providers-lenovo-470f5d68672b

Note: The benchmarks for this change do NOT include the changes from other PRs in the links below. Benchmarks of all changes can be found here.

Links

Partially addresses https://bugzilla.redhat.com/show_bug.cgi?id=1569626
Full collection of proposed changes: git compare
Document keeping track of overall improvements for this effort: gist
Related PRs:
Extracted PRs

The MiqPreloader.preload call was added prior to the usage of `.writable_storages` for the miq_request_workflow.rb. When writable_storages was called, it would make a new call to the database every time for each of the storages, which was not ideal. This change makes it so that the writable_storages method will first check to see if the proper data is already loaded, and if so, just use that. Otherwise, it will do the query as normal. The other change was to change the MiqPreloader.preload to include the `host_storages`, which properly applies the loaded records needed for the changes to the `host` model to work.

`.load_ar_obj` will defer to `.load_ar_objs` when it is passed an array, which will do a `.collect` call to `load_ar_obj` in return. This is a typical N+1, so this change takes the constantize code from `.load_ar_obj` and applies it to the `allowed_hosts` to make a single SQL call to for the needed hosts.

miq-bot · 2018-04-26T21:06:32Z

Checked commits NickLaMuro/manageiq@b77f060~...7bc4621 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 7 offenses detected

app/models/miq_request_workflow.rb

❗ - Line 942, Col 9 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

app/models/mixins/relationship_mixin.rb

❗ - Line 518, Col 91 - Style/ClassCheck - Prefer Object#kind_of? over Object#is_a?.

lib/miq_preloader.rb

⚠️ - Line 69, Col 115 - Lint/UnusedBlockArgument - Unused block argument - k. If it's necessary, use _ or _k as an argument name to indicate that it won't be used.
⚠️ - Line 69, Col 59 - Lint/UnusedBlockArgument - Unused block argument - rs. If it's necessary, use _ or _rs as an argument name to indicate that it won't be used.
⚠️ - Line 81, Col 33 - Lint/AssignmentInCondition - Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
❗ - Line 50, Col 33 - Style/BlockDelimiters - Avoid using {...} for multi-line blocks.
❗ - Line 79, Col 15 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

NickLaMuro · 2018-04-27T17:50:57Z

@miq-bot assign @gmcculloug
@miq-bot add_label performance

syncrou

@NickLaMuro
I get the assignment to (GM) due to changes in miq_request_workflow and miq_request_virt_workflow. I have 1 small comment, but otherwise things look good.

I'm not sure why the changes to relationship_mixin.rb, relationship.rb, and miq_preloader.rb are part of this PR. I think the changes in those 3 files should be viewed by a larger audience.

syncrou · 2018-05-16T17:35:14Z

app/models/miq_provision_virt_workflow.rb

@@ -1077,7 +1077,9 @@ def get_selected_hosts(src)
            elsif src[:host_id]
              selected_host(src)
            else
-              load_ar_obj(allowed_hosts)
+              allowed_hosts.group_by(&:evm_object_class).flat_map do |type, objs|


@NickLaMuro - do you think there might be a way to add the group_by behavior into load_ar_obj?

@syncrou Yeah, I was thinking about that. I mostly decided to shy away from that approach since isolating the code to this method would avoid potential risk of an unknown edge case of load_ar_obj that I was unware of.

That said, if you would prefer me trying to make this code work there, I can see what I can do.

@NickLaMuro - I would. Largely because that method is already abstracting out other potential edge cases. It should be easier to test the changes when applied to that method as well.

Since this go merged, I am going to address this specific case separately in another PR.

NickLaMuro · 2018-05-16T19:46:19Z

Thanks for the review, I appreciate it!

I'm not sure why the changes to relationship_mixin.rb, relationship.rb, and miq_preloader.rb are part of this PR. I think the changes in those 3 files should be viewed by a larger audience.

The main reason is the fulltree_opts portion of the diff requires the three of those changes as part of the optimization (see the last commit for more details).

That said, I agree with your statement that those files feel pretty out of place compared to the rest of the changes. I just figured it made sense to include all of the changes in a single digestible commit instead of splitting things up into two more additional PRs:

One to introduce the MiqPreloader changes
One to integrate those changes into the Relationship code
One to integrate the above into this PR

Though, it would pad my PR stats a bit more... 🤔

Not opposed to moving that last commit out of this PR and try to integrate that later. That said, I think the last commit has the biggest impact over all in terms of N+1s in this PR. Thoughts?

syncrou · 2018-05-17T18:54:06Z

I think a larger audience should have access to the miq-preloader (and relationship / relationship_mixin ) changes. Pulling those out of this PR will allow that to happen while we can move forward with the other changes here.

NickLaMuro · 2018-05-18T17:00:03Z

For posterity, these are the new metrics I was working up after removing the last commit in this PR:

	ms	queries	query (ms)	rows
before	15023	1961	873.4	48395
after	12149	709	566.1	48395

Before                                                     After
------                                                     -----

Total allocated: 2069091751 bytes (23226536 objects)   |   Total allocated: 2012882674 bytes (22396387 objects)
                                                       |   
allocated objects by gem                               |   allocated objects by gem
-----------------------------------                    |   -----------------------------------
   9665227  pending                                    |      9665227  pending
   5561668  manageiq/app                               |      5560427  manageiq/app
   3513258  manageiq/lib                               |      3513288  manageiq/lib
   2512007  activerecord-5.0.7                         |      1909097  activerecord-5.0.7
    861270  activesupport-5.0.7                        |       830581  activesupport-5.0.7
    418737  ancestry-2.2.2                             |       418737  ancestry-2.2.2
    278793  activemodel-5.0.7                          |       252515  activemodel-5.0.7
    178419  ruby-2.3.3/lib                             |       121139  arel-7.1.4
    165577  arel-7.1.4                                 |       108860  ruby-2.3.3/lib
     52875  manageiq-providers-vmware-0be2f13a0dc9     |        14424  fast_gettext-1.2.0
     14424  fast_gettext-1.2.0                         |         1609  other
      4115  other                                      |          318  manageiq-providers-vmware/app
       ...                                             |          ...

Of note, this doesn't include the changes that I was working on for load_ar_obj, but that will be addressed in another PR now.

NickLaMuro · 2018-05-31T01:16:58Z

@miq-bot add_label fine/yes, gaprindashvili/yes

…quest_workflow Prevents N+1 SQL queries in miq_request_workflow.rb (cherry picked from commit 65631b8) https://bugzilla.redhat.com/show_bug.cgi?id=1593798

simaishi · 2018-06-21T18:32:47Z

Fine backport details:

$ git log -1
commit 3f5401bf13b776226e243f4eba3b336c9f61866a
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Fri May 18 12:08:20 2018 -0400

    Merge pull request #17354 from NickLaMuro/sql_improvements_for_miq_request_workflow
    
    Prevents N+1 SQL queries in miq_request_workflow.rb
    (cherry picked from commit 65631b83ab169071c9691b130da84112de477ef6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593798

…quest_workflow Prevents N+1 SQL queries in miq_request_workflow.rb (cherry picked from commit 65631b8) https://bugzilla.redhat.com/show_bug.cgi?id=1593797

simaishi · 2018-06-21T19:04:32Z

Gaprindashvili backport details:

$ git log -1
commit 258b588a68c8b9dac0da056f2faba6efb3deed5a
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Fri May 18 12:08:20 2018 -0400

    Merge pull request #17354 from NickLaMuro/sql_improvements_for_miq_request_workflow
    
    Prevents N+1 SQL queries in miq_request_workflow.rb
    (cherry picked from commit 65631b83ab169071c9691b130da84112de477ef6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593797

NickLaMuro added 2 commits April 26, 2018 15:38

miq-bot assigned gmcculloug Apr 27, 2018

miq-bot added the performance label Apr 27, 2018

This was referenced May 3, 2018

Load Lans effeciently in MiqProvisionVirtWorkflow #17381

Merged

Avoid duplicate host load in allowed_hosts_obj #17402

Merged

[WIP] Lighter unique Host#domain lookup for in MiqProvisionVirtWorkflow #17403

Closed

NickLaMuro mentioned this pull request May 14, 2018

Add uniq on datacenters in #host_to_folder #17422

Merged

syncrou suggested changes May 16, 2018

View reviewed changes

NickLaMuro force-pushed the sql_improvements_for_miq_request_workflow branch from 7bc4621 to d986cb6 Compare May 17, 2018 21:57

kbrock merged commit 65631b8 into ManageIQ:master May 18, 2018

kbrock assigned kbrock and unassigned gmcculloug May 18, 2018

kbrock added this to the Sprint 86 Ending May 21, 2018 milestone May 18, 2018

This was referenced May 21, 2018

Use MiqPreloader.polymorphic_preload_for_child_classes in RelationshipMixin#fulltree_arranged #17460

Closed

Use MiqPreloader/Relationship improvements in MiqRequestWorkflow #17461

Closed

Add MiqPreloader.polymorphic_preload_for_child_classes #17457

Closed

miq-bot added the fine/yes label May 31, 2018

miq-bot added the gaprindashvili/yes label May 31, 2018

simaishi added fine/backported and removed fine/yes labels Jun 21, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jun 21, 2018

NickLaMuro mentioned this pull request Jan 24, 2019

Add method for restricting to accessible storages #18391

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevents N+1 SQL queries in miq_request_workflow.rb #17354

Prevents N+1 SQL queries in miq_request_workflow.rb #17354

NickLaMuro commented Apr 26, 2018 •

edited

Loading

miq-bot commented Apr 26, 2018

NickLaMuro commented Apr 27, 2018

syncrou left a comment

syncrou May 16, 2018

NickLaMuro May 16, 2018

syncrou May 17, 2018

NickLaMuro May 18, 2018

NickLaMuro commented May 16, 2018

syncrou commented May 17, 2018

NickLaMuro commented May 18, 2018

NickLaMuro commented May 31, 2018

simaishi commented Jun 21, 2018

simaishi commented Jun 21, 2018

Prevents N+1 SQL queries in miq_request_workflow.rb #17354

Prevents N+1 SQL queries in miq_request_workflow.rb #17354

Conversation

NickLaMuro commented Apr 26, 2018 • edited Loading

Benchmarks

Links

miq-bot commented Apr 26, 2018

NickLaMuro commented Apr 27, 2018

syncrou left a comment

Choose a reason for hiding this comment

syncrou May 16, 2018

Choose a reason for hiding this comment

NickLaMuro May 16, 2018

Choose a reason for hiding this comment

syncrou May 17, 2018

Choose a reason for hiding this comment

NickLaMuro May 18, 2018

Choose a reason for hiding this comment

NickLaMuro commented May 16, 2018

syncrou commented May 17, 2018

NickLaMuro commented May 18, 2018

NickLaMuro commented May 31, 2018

simaishi commented Jun 21, 2018

simaishi commented Jun 21, 2018

NickLaMuro commented Apr 26, 2018 •

edited

Loading