From 4f361f2d5424dcac346a3e7fb9cfb4dfd97ddada Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 24 Apr 2018 17:41:25 -0500 Subject: [PATCH 1/6] Patch Ancestry::InstanceMethods#cast_primary_key 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 ... | ... --- app/models/orchestration_stack.rb | 2 ++ app/models/relationship.rb | 1 + app/models/service.rb | 1 + app/models/tenant.rb | 1 + lib/patches/ancestry_patch.rb | 13 +++++++++++++ 5 files changed, 18 insertions(+) create mode 100644 lib/patches/ancestry_patch.rb diff --git a/app/models/orchestration_stack.rb b/app/models/orchestration_stack.rb index a74d36ab56f..4c7b333e4aa 100644 --- a/app/models/orchestration_stack.rb +++ b/app/models/orchestration_stack.rb @@ -1,4 +1,6 @@ require 'ancestry' +require 'ancestry_patch' + class OrchestrationStack < ApplicationRecord require_nested :Status diff --git a/app/models/relationship.rb b/app/models/relationship.rb index 08bd479049c..de26c9aaedf 100644 --- a/app/models/relationship.rb +++ b/app/models/relationship.rb @@ -1,4 +1,5 @@ require 'ancestry' +require 'ancestry_patch' class Relationship < ApplicationRecord has_ancestry diff --git a/app/models/service.rb b/app/models/service.rb index 0ff733d5789..371969597a9 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -1,4 +1,5 @@ require 'ancestry' +require 'ancestry_patch' class Service < ApplicationRecord DEFAULT_PROCESS_DELAY_BETWEEN_GROUPS = 120 diff --git a/app/models/tenant.rb b/app/models/tenant.rb index 851cb12c25c..b0b3e5f59b6 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -1,4 +1,5 @@ require 'ancestry' +require 'ancestry_patch' class Tenant < ApplicationRecord HARDCODED_LOGO = "custom_logo.png" diff --git a/lib/patches/ancestry_patch.rb b/lib/patches/ancestry_patch.rb new file mode 100644 index 00000000000..e731caaa136 --- /dev/null +++ b/lib/patches/ancestry_patch.rb @@ -0,0 +1,13 @@ +module Ancestry + module InstanceMethods + STRING_BASED_KEYS = %i[string uuid text].freeze + + def cast_primary_key(key) + if STRING_BASED_KEYS.include?(primary_key_type) + key + else + key.to_i + end + end + end +end From ae2dc07968e194889d8d73189df619193852dcb0 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 24 Apr 2018 18:05:37 -0500 Subject: [PATCH 2/6] Patch Ancestry::InstanceMethods#parse_ancestry_column 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 <<<<<<<<<< ... | ... --- lib/patches/ancestry_patch.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/patches/ancestry_patch.rb b/lib/patches/ancestry_patch.rb index e731caaa136..148d2f92169 100644 --- a/lib/patches/ancestry_patch.rb +++ b/lib/patches/ancestry_patch.rb @@ -1,7 +1,10 @@ module Ancestry module InstanceMethods - STRING_BASED_KEYS = %i[string uuid text].freeze + def parse_ancestry_column obj + obj.to_s.split('/').map! { |id| cast_primary_key(id) } + end + STRING_BASED_KEYS = %i[string uuid text].freeze def cast_primary_key(key) if STRING_BASED_KEYS.include?(primary_key_type) key From f795d91d49d145754e2522f1034f97cbbb533e4c Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 24 Apr 2018 18:12:52 -0500 Subject: [PATCH 3/6] Patch Ancestry::InstanceMethods#ancestor_ids 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 <<<<<<<< ... | ... --- lib/patches/ancestry_patch.rb | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/lib/patches/ancestry_patch.rb b/lib/patches/ancestry_patch.rb index 148d2f92169..85285d108ec 100644 --- a/lib/patches/ancestry_patch.rb +++ b/lib/patches/ancestry_patch.rb @@ -1,9 +1,27 @@ +module AncestryInstanceMethodsPatch + def update_descendants_with_new_ancestry + super + unless ancestry_callbacks_disabled? + clear_memoized_instance_variables + if ancestry_changed? && !new_record? && sane_ancestry? + unscoped_descendants.each(&:clear_memoized_instance_variables) + end + end + end +end + module Ancestry module InstanceMethods - def parse_ancestry_column obj + prepend AncestryInstanceMethodsPatch + + def parse_ancestry_column(obj) obj.to_s.split('/').map! { |id| cast_primary_key(id) } end + def ancestor_ids + @_ancestor_ids ||= parse_ancestry_column(read_attribute(ancestry_base_class.ancestry_column)) + end + STRING_BASED_KEYS = %i[string uuid text].freeze def cast_primary_key(key) if STRING_BASED_KEYS.include?(primary_key_type) @@ -12,5 +30,9 @@ def cast_primary_key(key) key.to_i end end + + def clear_memoized_instance_variables + @_ancestor_ids = nil + end end end From 870dde81a46ad1174731b3bc4cc684c4c58961ae Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Mon, 14 May 2018 12:16:12 -0500 Subject: [PATCH 4/6] Add ANCESTRY_DELIMITER to ancestry_patch.rb 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. --- lib/patches/ancestry_patch.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/patches/ancestry_patch.rb b/lib/patches/ancestry_patch.rb index 85285d108ec..39fca5db778 100644 --- a/lib/patches/ancestry_patch.rb +++ b/lib/patches/ancestry_patch.rb @@ -14,8 +14,10 @@ module Ancestry module InstanceMethods prepend AncestryInstanceMethodsPatch + ANCESTRY_DELIMITER = '/'.freeze + def parse_ancestry_column(obj) - obj.to_s.split('/').map! { |id| cast_primary_key(id) } + obj.to_s.split(ANCESTRY_DELIMITER).map! { |id| cast_primary_key(id) } end def ancestor_ids From c2be7c2e94e6c99145f919d6b49c2d1e05e8cf65 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Mon, 14 May 2018 12:17:28 -0500 Subject: [PATCH 5/6] Optimize `#depth` for Ancestry instance_methods 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. --- lib/patches/ancestry_patch.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/patches/ancestry_patch.rb b/lib/patches/ancestry_patch.rb index 39fca5db778..5e9304bdbeb 100644 --- a/lib/patches/ancestry_patch.rb +++ b/lib/patches/ancestry_patch.rb @@ -24,6 +24,15 @@ def ancestor_ids @_ancestor_ids ||= parse_ancestry_column(read_attribute(ancestry_base_class.ancestry_column)) end + def depth + @_depth ||= if @_ancestor_ids + @_ancestor_ids.size + else + col = read_attribute(ancestry_base_class.ancestry_column) + col ? col.count(ANCESTRY_DELIMITER) + 1 : 0 + end + end + STRING_BASED_KEYS = %i[string uuid text].freeze def cast_primary_key(key) if STRING_BASED_KEYS.include?(primary_key_type) @@ -35,6 +44,7 @@ def cast_primary_key(key) def clear_memoized_instance_variables @_ancestor_ids = nil + @_depth = nil end end end From c0b8b69f622ea59ed252430eda55a6dff6152ba8 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Mon, 14 May 2018 12:23:18 -0500 Subject: [PATCH 6/6] Optimize `#parent_id` Ancestry instance_method 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. --- lib/patches/ancestry_patch.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/patches/ancestry_patch.rb b/lib/patches/ancestry_patch.rb index 5e9304bdbeb..c8888d34676 100644 --- a/lib/patches/ancestry_patch.rb +++ b/lib/patches/ancestry_patch.rb @@ -24,6 +24,23 @@ def ancestor_ids @_ancestor_ids ||= parse_ancestry_column(read_attribute(ancestry_base_class.ancestry_column)) end + def parent_id + return @_parent_id if defined?(@_parent_id) + @_parent_id = if @_ancestor_ids + @_ancestor_ids.empty? ? nil : @_ancestor_ids.last + else + 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 + nil + else + rindex = col.rindex(ANCESTRY_DELIMITER) + cast_primary_key(rindex ? col[rindex + 1, col.length] : col) + end + end + end + def depth @_depth ||= if @_ancestor_ids @_ancestor_ids.size @@ -45,6 +62,9 @@ def cast_primary_key(key) def clear_memoized_instance_variables @_ancestor_ids = nil @_depth = nil + + # can't assign to `nil` since `nil` could be a valid result + remove_instance_variable(:@_parent_id) if defined?(@_parent_id) end end end