Ancestry Patches #17360

NickLaMuro · 2018-04-27T05:12:55Z

Various patches to ancestry to help with performance of miq_request_workflow.rb. Will eventually work them into the main project, but for now, this is the best way to introduce the changes without needing to bump the version of ancestry in the process.

Benchmarks

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	13727	1961	48395

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

Total allocated: 2069091751 bytes (23226536 objects)   |   Total allocated: 2051853547 bytes (22893961 objects)
                                                       |   
allocated objects by gem                               |   allocated objects by gem
-----------------------------------                    |   -----------------------------------
   9665227  pending                                    |      9665227  pending
   5561668  manageiq/app                               |      5561669  manageiq/app
   3513258  manageiq/lib                               |      3581556  manageiq/lib
   2512007  activerecord-5.0.7                         |      2494275  activerecord-5.0.7
    861270  activesupport-5.0.7                        |       861287  activesupport-5.0.7
    418737  ancestry-2.2.2  <<<<<<<<<<                 |       278793  activemodel-5.0.7
    278793  activemodel-5.0.7                          |       178419  ruby-2.3.3/lib
    178419  ruby-2.3.3/lib                             |       165577  arel-7.1.4
    165577  arel-7.1.4                                 |        52875  manageiq-providers-vmware-0be2f13a0dc9
     52875  manageiq-providers-vmware-0be2f13a0dc9     |        35578  ancestry-2.2.2  <<<<<<<<<<
     14424  fast_gettext-1.2.0                         |        14424  fast_gettext-1.2.0
       ...                                             |          ...

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

BACKPORT Ancestry Patch updates/fixes #17511 WITH THIS PR!
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:

NickLaMuro · 2018-04-27T17:51:51Z

Looking into the test failures on this one. Unsure what the issue is.

NickLaMuro · 2018-04-27T21:34:49Z

Failure is due to the memoization of @_ancestor_ids and changing the ancestor location. Looking into a way of making still work, but might have to remove it.

Thank god for the tests around that though...

Prevents a new version of the STRING_BASED_KEYS array from being instantiated every time that cast_primary_key is called. * * * Before/After ------------ Total allocated: 324542397 bytes (3095350 objects) | Total allocated: 318431331 bytes (2943673 objects) | allocated objects by gem | allocated objects by gem ----------------------------------- | ----------------------------------- 1576834 activerecord-5.0.7 | 1576834 activerecord-5.0.7 477120 manageiq/app | 477120 manageiq/app 418737 ancestry-2.2.2 <<<<<<<<<< | 274449 activemodel-5.0.7 274449 activemodel-5.0.7 | 267060 ancestry-2.2.2 <<<<<<<<<< 106559 activesupport-5.0.7 | 106559 activesupport-5.0.7 82799 pending | 82799 pending 74117 ruby-2.3.3/lib | 74117 ruby-2.3.3/lib ... | ...

Calls `.map!` instead of `.map` since we already have a temp array from the `.split`, and this avoids creating a completely new array for the second time in this method. * * * Before/After ------------ Total allocated: 318431331 bytes (2943673 objects) | Total allocated: 316196585 bytes (2917071 objects) | allocated objects by gem | allocated objects by gem ----------------------------------- | ----------------------------------- 1576834 activerecord-5.0.7 | 1576834 activerecord-5.0.7 477120 manageiq/app | 477120 manageiq/app 274449 activemodel-5.0.7 | 274449 activemodel-5.0.7 267060 ancestry-2.2.2 <<<<<<<<<< | 208479 manageiq/lib 106559 activesupport-5.0.7 | 106557 activesupport-5.0.7 82799 pending | 82799 pending 74117 ruby-2.3.3/lib | 74117 ruby-2.3.3/lib 52875 manageiq-providers-vmware-0be2f13a0dc9 | 52875 manageiq-providers-vmware-0be2f13a0dc9 14424 fast_gettext-1.2.0 | 35578 ancestry-2.2.2 <<<<<<<<<< ... | ...

In many methods, this is generated twice because the result is never saved. This simply adds memoization to the result of this method. Of Note: The extra `#clear_memoized_instance_variables` additions were necessary to fix failing tests due to ancestry records being updated and saved, and the `@_ancestor_ids` value became out of date. * * * Before/After ------------ **Note:** Only the totals here change... which is a bit weird and I didn't notice it before (probably just a reporting error on MemoryProfiler's end), but this patch was the only thing changed between the two runs. Total allocated: 316196585 bytes (2917071 objects) | Total allocated: 307262021 bytes (2762757 objects) ^^^^^^^ | ^^^^^^^ | allocated objects by gem | allocated objects by gem ----------------------------------- | ----------------------------------- 1576834 activerecord-5.0.7 | 1559102 activerecord-5.0.7 477120 manageiq/app | 477120 manageiq/app 274449 activemodel-5.0.7 | 274449 activemodel-5.0.7 208479 manageiq/lib | 106559 activesupport-5.0.7 106557 activesupport-5.0.7 | 82799 pending 82799 pending | 74117 ruby-2.3.3/lib 74117 ruby-2.3.3/lib | 71895 manageiq/lib 52875 manageiq-providers-vmware-0be2f13a0dc9 | 52875 manageiq-providers-vmware-0be2f13a0dc9 35578 ancestry-2.2.2 <<<<<<<<<< | 35578 ancestry-2.2.2 <<<<<<<< ... | ...

Makes '/' a constant so it isn't allocated every time `parse_ancestry_column` is called. A minor improvement here, but will help with future commits.

NickLaMuro · 2018-05-22T00:13:13Z

@miq-bot assign @kbrock
@miq-bot add_label performance

kbrock

I just found that one thing.

Thanks for helping get ancestry better

kbrock · 2018-05-29T18:33:45Z

lib/patches/ancestry_patch.rb

+    def parent_id
+      return @_parent_id if defined?(@_parent_id)
+      @_parent_id = if @ancestor_ids
+                      @ancestor_ids.empty? ? nil : @ancestor_ids.last


a) think we need @_ancestor_ids with the underscore

Are we expecting to only call parent_id and not use any other ancestor_ids methods?

Oh butts... you are totally right. Good catch.

Wonder if that affected my metrics at all. 🤔

kbrock · 2018-05-29T18:34:12Z

lib/patches/ancestry_patch.rb

+                      col = read_attribute(ancestry_base_class.ancestry_column)
+                      # Specifically not using `.blank?` here because it is
+                      # slower than doing the below.
+                      if col.nil? || col.empty? # rubocop:disable Rails/Blank


I think I like col&.empty? - will remove the rails/blank comments and is faster

so tell me. How do you really feel about &.?

kbrock · 2018-05-30T14:08:19Z

lib/patches/ancestry_patch.rb

+    end
+
+    STRING_BASED_KEYS = %i[string uuid text].freeze
+    def cast_primary_key(key)


curious if the new casting changes are good enough to make this not necessary anymore

curious if the new casting changes are good enough to make this not necessary anymore

Possible. There are definitely things that are now obsolete in this patch that are fixed upstream.

That said, these are meant to be the minimal viable changes that can be shipped with MIQ with the current version of ancestry that we are using. My guess is that for backporting, we can't/shouldn't upgrade the version of ancestry we are using.

Not saying we shouldn't upgrade ancestry in master, but I see it as not feasible/safe to take what has been done in the gem and just merge it in for a backport. This patch is intended for fine/gaprindashvili.

If @ancestor_ids has already been calculated, using that value will be much more efficient for calculating the depth since the work to do so has already been done. But if it hasn't (for example, when calling `.arrange_nodes`), then ancestor_ids is never actually needed in full, and we can avoid the object allocations and CPU cycles needed to determine the depth and simply count the number of instances of '/' in the string using `String#count` (doesn't allocate any objects in doing so). `@_depth`, like `@_ancestor_ids`, needs to be reset if the tree is changed.

If @ancestor_ids has already been calculated, using that value will be much more efficient for calculating the parent since the work translate the string to the proper typecasted value has already been done. But if it hasn't (for example, when calling `.arrange_nodes`), then ancestor_ids is never actually needed in full, and we can avoid the object allocations and CPU cycles needed to determine the depth by typecasting all of the ids in the string and just use a bit of substring manipulation to only allocate one object prior to typecasting (instead of N+1 for number of ancestors). Also, since it is possible for `@_parent_id` to be `nil`, make sure we are removing the instance variable instead of just setting it to `nil` if the location in the tree has been changed.

miq-bot · 2018-05-30T16:22:54Z

Checked commits NickLaMuro/manageiq@4f361f2~...c0b8b69 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

lib/patches/ancestry_patch.rb

❗ - Line 53, Col 25 - Style/PercentLiteralDelimiters - %i-literals should be delimited by ( and ).

NickLaMuro · 2018-05-31T01:11:50Z

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

NickLaMuro · 2018-05-31T01:13:02Z

^ I see the above labels as possibly debatable. I can work with whoever is needed on deciding what we want to and not want to backport from this PR.

ZitaNemeckova · 2018-05-31T11:26:07Z

FYI this broke-ish (it broke tests for dead code) UI master :)

  1) OpsController::Tenants #tenant_set_record_vars sets value of parent & divisible fields for new record
     Failure/Error: expect(tenant.parent.id).to eq(@tenant.id)
       expected: 13000000002204
            got: 13000000002203
       (compared using ==)
     # ./spec/controllers/ops_controller/ops_rbac_spec.rb:277:in `block (4 levels) in <top (required)>'

Fixed in ManageIQ/manageiq-ui-classic#4011

@simaishi please backport this with that UI PR ☝️ Thanks :)

@NickLaMuro For Gaprindashvili it's ok but for Fine adding new Tenant will be broken because it will not be saved with correct parent. If this needs to be backported to Fine it will need an UI PR that fixes it.

NickLaMuro · 2018-05-31T14:15:16Z

@ZitaNemeckova Thanks!

I will look into that then, and also update the description regarding your fix for master/gaprindashvili.

ZitaNemeckova · 2018-05-31T14:27:04Z

If it helps this line is the problem.

Edit: Fixed the link. Thanks Nick.

gmcculloug · 2018-06-02T15:56:41Z

@NickLaMuro Tracked down failing tests in https://github.com/ManageIQ/manageiq-automation_engine#manageiq-automation-engine to this PR.

Tests started failing here: https://travis-ci.org/ManageIQ/manageiq-automation_engine/jobs/385973677

Any help would be appreciated.
cc @mkanoor

NickLaMuro · 2018-06-02T16:14:27Z

Sorry about that... taking a look now.

NickLaMuro · 2018-06-02T16:46:34Z

Correction to @ZitaNemeckova 's link, I believe that this line is what is what was causing the issue (made it a permalink instead of pointing to master).

NickLaMuro · 2018-06-02T20:25:11Z

So wanted to give an update because this is failing in more than one place now... 😞

So the issue in both cases is due to the memoization be held on for the variables after something in the tree structure has changed. I put in a piece of code to handle this:

https://github.com/ManageIQ/manageiq/blob/a49e48a4/lib/patches/ancestry_patch.rb#L1-L11

But it looks like it is isn't getting fired correctly in the specs when things change. Possibly due to the fact that the records we are working with haven't been saved to the DB, and there are specs that work against Tenant records (which use ancestry) that are only built up as unsaved records (I think...).

With manageiq-ui-classic, I think I was able to fix things by adding the following:

module AncestryInstanceMethodsPatch
  def update_descendants_with_new_ancestry
    # ...
  end

  def parent=(parent)
    super
    update_descendants_with_new_ancestry
  end

  def parent_id=(parent_id)
    super
    update_descendants_with_new_ancestry
  end
end

Which will clear out the instance variable @_parent_id when parent_id is updated (and it had been set prior). The problem with manageiq-automation_engine however is with the ancestor_ids method, and not with the parent_id method, so I still have to figure out what is going wrong there.

Removing all of the memoization fixes everything just fine, but I am trying to avoid doing that if I can help it. I will update later when I think I have a better fix in place for both, or have more to share.

NickLaMuro · 2018-06-02T20:48:55Z

Figured it out! Patch to fix incoming.

NickLaMuro · 2018-06-02T21:15:20Z

Here is the patch:

#17511

This patch should remove the need to backport ManageIQ/manageiq-ui-classic#4011 along with this PR to gaprindashvili (though, probably wouldn't hurt). Also should fix the issues that @gmcculloug was seeing in the manageiq-automation_engine repo.

gmcculloug · 2018-06-03T00:37:30Z

Tests are passing locally for me with this change in place. Thanks @NickLaMuro. 👍

Ancestry Patches (cherry picked from commit 095f4dc) https://bugzilla.redhat.com/show_bug.cgi?id=1593798

simaishi · 2018-06-21T18:34:48Z

Fine backport details:

$ git log -1
commit f92ee1f15fd14b953aeed3ff8aaf73a29723000e
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Wed May 30 14:46:15 2018 -0400

    Merge pull request #17360 from NickLaMuro/ancestry_patches
    
    Ancestry Patches
    (cherry picked from commit 095f4dce436fa9f9fdedca852b53c55c5f6cb6e0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593798

Ancestry Patches (cherry picked from commit 095f4dc) https://bugzilla.redhat.com/show_bug.cgi?id=1593797

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

Gaprindashvili backport details:

$ git log -1
commit 065dd8dd25ab563b831188b30515de233634eba8
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Wed May 30 14:46:15 2018 -0400

    Merge pull request #17360 from NickLaMuro/ancestry_patches
    
    Ancestry Patches
    (cherry picked from commit 095f4dce436fa9f9fdedca852b53c55c5f6cb6e0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593797

NickLaMuro force-pushed the ancestry_patches branch 2 times, most recently from df6cc28 to 0e6173d Compare April 27, 2018 22:56

NickLaMuro mentioned this pull request May 3, 2018

Load Lans effeciently in MiqProvisionVirtWorkflow #17381

Merged

NickLaMuro mentioned this pull request May 14, 2018

Add uniq on datacenters in #host_to_folder #17422

Merged

NickLaMuro added 4 commits May 15, 2018 12:11

Add ANCESTRY_DELIMITER to ancestry_patch.rb

870dde8

Makes '/' a constant so it isn't allocated every time `parse_ancestry_column` is called. A minor improvement here, but will help with future commits.

NickLaMuro force-pushed the ancestry_patches branch 3 times, most recently from 51fcab2 to b7e3d2d Compare May 17, 2018 16:06

NickLaMuro mentioned this pull request May 21, 2018

Use MiqPreloader/Relationship improvements in MiqRequestWorkflow #17461

Closed

miq-bot assigned kbrock May 22, 2018

miq-bot added the performance label May 22, 2018

kbrock reviewed May 30, 2018

View reviewed changes

NickLaMuro added 2 commits May 30, 2018 11:19

NickLaMuro force-pushed the ancestry_patches branch from b7e3d2d to c0b8b69 Compare May 30, 2018 16:20

kbrock merged commit 095f4dc into ManageIQ:master May 30, 2018

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

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

ZitaNemeckova mentioned this pull request May 31, 2018

Remove dead code related to Tenant ManageIQ/manageiq-ui-classic#4011

Merged

NickLaMuro mentioned this pull request Jun 2, 2018

Ancestry Patch updates/fixes #17511

Merged

agrare added this to the Sprint 87 Ending Jun 4, 2018 milestone Jun 4, 2018

simaishi pushed a commit that referenced this pull request Jun 21, 2018

Merge pull request #17360 from NickLaMuro/ancestry_patches

f92ee1f

Ancestry Patches (cherry picked from commit 095f4dc) https://bugzilla.redhat.com/show_bug.cgi?id=1593798

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

simaishi pushed a commit that referenced this pull request Jun 21, 2018

Merge pull request #17360 from NickLaMuro/ancestry_patches

065dd8d

Ancestry Patches (cherry picked from commit 095f4dc) https://bugzilla.redhat.com/show_bug.cgi?id=1593797

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

kbrock mentioned this pull request Oct 12, 2018

Rails 5.0/5.1: Update to ancestry 3.0.0+ for rails 5.1 fix #18082

Closed

kbrock mentioned this pull request Aug 18, 2020

remove ancestry patch #20431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ancestry Patches #17360

Ancestry Patches #17360

NickLaMuro commented Apr 27, 2018 •

edited

Loading

NickLaMuro commented Apr 27, 2018

NickLaMuro commented Apr 27, 2018 •

edited

Loading

NickLaMuro commented May 22, 2018

kbrock left a comment

kbrock May 29, 2018

NickLaMuro May 30, 2018

kbrock May 29, 2018

NickLaMuro May 30, 2018

kbrock May 30, 2018

kbrock May 30, 2018

NickLaMuro May 30, 2018

miq-bot commented May 30, 2018

NickLaMuro commented May 31, 2018

NickLaMuro commented May 31, 2018

ZitaNemeckova commented May 31, 2018 •

edited

Loading

NickLaMuro commented May 31, 2018

ZitaNemeckova commented May 31, 2018 •

edited

Loading

gmcculloug commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018 •

edited

Loading

gmcculloug commented Jun 3, 2018

simaishi commented Jun 21, 2018

simaishi commented Jun 21, 2018

Ancestry Patches #17360

Ancestry Patches #17360

Conversation

NickLaMuro commented Apr 27, 2018 • edited Loading

Benchmarks

Links

NickLaMuro commented Apr 27, 2018

NickLaMuro commented Apr 27, 2018 • edited Loading

NickLaMuro commented May 22, 2018

kbrock left a comment

Choose a reason for hiding this comment

kbrock May 29, 2018

Choose a reason for hiding this comment

NickLaMuro May 30, 2018

Choose a reason for hiding this comment

kbrock May 29, 2018

Choose a reason for hiding this comment

NickLaMuro May 30, 2018

Choose a reason for hiding this comment

kbrock May 30, 2018

Choose a reason for hiding this comment

kbrock May 30, 2018

Choose a reason for hiding this comment

NickLaMuro May 30, 2018

Choose a reason for hiding this comment

miq-bot commented May 30, 2018

NickLaMuro commented May 31, 2018

NickLaMuro commented May 31, 2018

ZitaNemeckova commented May 31, 2018 • edited Loading

NickLaMuro commented May 31, 2018

ZitaNemeckova commented May 31, 2018 • edited Loading

gmcculloug commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018

NickLaMuro commented Jun 2, 2018 • edited Loading

gmcculloug commented Jun 3, 2018

simaishi commented Jun 21, 2018

simaishi commented Jun 21, 2018

NickLaMuro commented Apr 27, 2018 •

edited

Loading

NickLaMuro commented Apr 27, 2018 •

edited

Loading

ZitaNemeckova commented May 31, 2018 •

edited

Loading

ZitaNemeckova commented May 31, 2018 •

edited

Loading

NickLaMuro commented Jun 2, 2018 •

edited

Loading