Use MiqPreloader/Relationship improvements in MiqRequestWorkflow

[NickLaMuro]

Extracted from #17354

Built off of and depends on #17457 and #17460

This takes the MiqPreloader.polymorphic_preload_for_child_classes changes, and makes use of them through the fulltree_arranged method to drastically reduce the number of N+1 queries in MiqRequestWorkflow#ems_metadata_tree.

Benchmarks

tl; dr Same as in the other PRs, just different code

Note: There is actually a slight improvement in number of queries called when combining both the changes from this branch, and the previous ones in #17354. This is due to the addition of Host => hosts_scope option which addresses a slightly smaller N+1 for hosts that happen to show up directly in the ems_metadata_tree versus are accessed through the EmsCluster records. In my dataset, this was only 4 hosts in total this affected, but in different scendarios, this might have a bigger affect.

Also, these benchmarks don't integrate the merged changes from #17354 as I have been benchmarking all of these changes from an individual perspective off of a specific commit from master, and wanted to keep the before and after benchmark results consistent with the rest of the related PRs.

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	query (ms)	rows
before	15023	1961	873.4	48395
after	12772	1308	451.2	48395

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

Total allocated: 2069091751 bytes (23226536 objects)   |   Total allocated: 2023972840 bytes (22605560 objects)
                                                       |   
allocated objects by gem                               |   allocated objects by gem
-----------------------------------                    |   -----------------------------------
   9665227  pending                                    |      9665227  pending
   5561668  manageiq/app  <<<<<<<<<<                   |      5559167  manageiq/app  <<<<<<<<<<
   3513258  manageiq/lib  <<<<<<<<<<                   |      3508927  manageiq/lib  <<<<<<<<<<
   2512007  activerecord-5.0.7  <<<<<<<<<<             |      2131310  activerecord-5.0.7  <<<<<<<<<<
    861270  activesupport-5.0.7 <<<<<<<<<<             |       839043  activesupport-5.0.7  <<<<<<<<<<
    418737  ancestry-2.2.2                             |       418737  ancestry-2.2.2
    278793  activemodel-5.0.7  <<<<<<<<<<              |       276326  activemodel-5.0.7  <<<<<<<<<<
    178419  ruby-2.3.3/lib  <<<<<<<<<<                 |        77456  ruby-2.3.3/lib  <<<<<<<<<<
    165577  arel-7.1.4  <<<<<<<<<<                     |        59093  arel-7.1.4  <<<<<<<<<<
     52875  manageiq-providers-vmware-0be2f13a0dc9     |        52875  manageiq-providers-vmware-0be2f13a0dc9
     14424  fast_gettext-1.2.0                         |        14424  fast_gettext-1.2.0
      4115  other  <<<<<<<<<<                          |         2809  other  <<<<<<<<<<
        75  default_value_for-3.0.5                    |           75  default_value_for-3.0.5
        68  concurrent-ruby-1.0.5                      |           68  concurrent-ruby-1.0.5
        16  memoist-0.15.0                             |           16  memoist-0.15.0
         4  rubygems                                   |            4  rubygems

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
• Extracted from Prevents N+1 SQL queries in miq_request_workflow.rb #17354
• Built off of and depends on:
  • Add MiqPreloader.polymorphic_preload_for_child_classes #17457
  • Use MiqPreloader.polymorphic_preload_for_child_classes in RelationshipMixin#fulltree_arranged #17460
• Full collection of proposed changes: git compare
• Document keeping track of overall improvements for this effort: gist
• Other Related PRs:
  • Memoize _log.prefix calls #17355
  • Use nested hashes instead of string keys #17357
  • Refactor get_ems_folders to create less strings #17358
  • Use OpenStruct over MiqHashStruct #17359
  • Ancestry Patches #17360
  • Load Lans effeciently in MiqProvisionVirtWorkflow #17381
  • Avoid duplicate host load in allowed_hosts_obj #17402
  • [WIP] Lighter unique Host#domain lookup for in MiqProvisionVirtWorkflow #17403
  • Add MiqPreloader.polymorphic_preload_for_child_classes #17457

[NickLaMuro]

@miq-bot add_label performance

@syncrou This is the follow-up PR which re-integrates the MiqPreloader work into the MiqRequestWorkflow. Didn't assign anyone because I wasn't sure who it should go to at this point, but at least keeping you in the loop since you did the initial review on #17354

cc @kbrock @Fryguy

[miq-bot]

Checked commits NickLaMuro/manageiq@2f971d5~...aaf21ce with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 4 offenses detected

lib/miq_preloader.rb

• ⚠️ - Line 77, 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 77, 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 58, Col 33 - Style/BlockDelimiters - Avoid using {...} for multi-line blocks.
• ❗ - Line 98, Col 15 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

[kbrock]

fyi: https://apidock.com/rails/ActiveSupport/CoreExtensions/Array/ExtractOptions/extract_options%21

[NickLaMuro]

I was aware of this, but I was avoiding it since down the chain it is also called in fulltree_rels_arranged. Instead of changing pulling out the options here, I was being explicit and only removing the single key I needed here. Seemed less risky to not mutate args here when it was already being done later in fulltree_rels_arranged.

That said, I am not terribly partial to this line of thought here. I guess the diff for doing this would then look like this:

   def fulltree_arranged(*args)
-    Relationship.arranged_rels_to_resources(fulltree_rels_arranged(*args))
+    options = args.extract_options!
+    class_specific_preloaders = options.delete(:class_specific_preloaders)
+    Relationship.arranged_rels_to_resources(fulltree_rels_arranged(*args, options), true, class_specific_preloaders)
   end

Does make that last line a bit longer, and I am all about shorter lines when possible, but should be the same.

[miq-bot]

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!