Add a cache for full Feature objects #14037

hayesr · 2017-02-23T00:15:31Z

This speeds up RBAC features tree significantly by adding cached methods that return full objects.

hayesr · 2017-02-23T19:24:18Z

@kbrock This is what I ended up doing to improve RBAC Features tree perf.

chessbyte · 2017-02-23T22:28:35Z

@kbrock @Fryguy @gtanzillo we need to review and potentially redesign this MiqProductFeature class vis-a-vis RBAC. It was originally created exclusively for the Classic UI. Now that we have REST API and the Service UI, it is time to re-think this.

Fryguy · 2017-02-24T02:16:05Z

app/models/miq_product_feature.rb

+  end
+
+  def self.obj_features
+    @obj_cache ||= begin


Could this cause test contamination? If so, can you add it to EvmSpecHelper.clear_caches so it will be cleared on every test automatically?

fixed below - thanks

kbrock · 2017-02-24T14:58:09Z

We already cache features, just not using AR objects.

Every time I've seen performance problems with features, it is because we were using traditional AR access into features instead of the feature_root/feature_children (or even feature_all_children)

Maybe now is the time to go from storing strings in a tree to storing a tree of AR objects?
Or maybe we just need to apply our previous handling of this to the ui

Fryguy · 2017-02-24T19:23:57Z

We already cache features, just not using AR objects.

Link plz?

kbrock · 2017-02-25T04:12:06Z

@Fryguy see above -- These methods all access MiqProductFeature#features, which is where this is cached.

History: I didn't come up with this caching mechanism, but I did fix a bunch of bugs and changed the n+1 query to a single query. @lgalis helped me track down nuances in some bugs. Also, @chrisarcand and @jrafanie helped when we were performing up to 3 feature queries on some pages. Not sure if they remember those.

tangent:

You know, since this data isn't changed and is shipped with the product, it seems odd that we are loading into the db, not editing, and bringing back to cache in memory locally. Gems like ActiveHash were made to solve this problem. (it just loads the data from yaml and skips the db)

kbrock · 2017-02-25T04:13:39Z

Also, for the record, we are not performing Rbac on features, but rather displaying this feature tree for an Rbac screen

chrisarcand · 2017-02-27T15:21:22Z

@Fryguy Link to current feature caching

kbrock

didn't mean to apply the brakes.

We've had to fix code because people were doing things more of an object/rails way.
Now is a good time to get closer to rails here

kbrock · 2017-02-27T16:36:51Z

app/models/miq_product_feature.rb

+  end
+
+  def self.obj_features
+    @obj_cache ||= begin


fixed below - thanks

kbrock · 2017-02-27T16:38:26Z

app/models/miq_product_feature.rb

+  def self.obj_features
+    @obj_cache ||= begin
+      # create hash with parent identifier and details
+      features = select('*').select(arel_attribute(:parent_identifier).as("parent_identifier")).each_with_object({}) do |f, h|


Think select api has been enhanced since this was written. Could you test with:

select('*').select(:parent_identifier).each_with_object({})

kbrock · 2017-02-27T16:39:44Z

app/models/miq_product_feature.rb

+    end
+  end
+
+  def self.obj_feature_all_children(identifier)


would have been nice if we didn't have to prefix with obj_

a followup PR may be able to remove the string/hash only methods

Agreed. I believe there are several places in both repos that lean on the hash method, but I'd be interested in moving that to objects instead.

kbrock · 2017-02-27T16:40:31Z

app/models/miq_product_feature.rb

+    feature = obj_features[identifier.to_s]
+    ancestors = []
+
+    if feature


maybe:

return [] unless feature

I think I would need an array-building alternative to using until, which I would prefer but haven't come up with yet.

I was talking about line 195... but it is minor

ooh, and in response to your comment:

def self.obj_feature_ancestors(identifier) feature = obj_features[identifier.to_s] return [] unless feature ancestors = [] while (prnt = obj_features[feature[:parent]].try(:[], :feature)) feature = obj_features[prnt.identifier] ancestors << prnt end ancestors end

An array building alternative could be to just use recursion.

def self.obj_feature_ancestors(identifier) feature = obj_features[identifier.to_s] return [] unless feature parent_feature = obj_features[feature[:parent]].try(:[], :feature) return [] unless parent_feature obj_feature_ancestors(parent_feature.identifier).unshift(parent_feature) end

This seems a bit more intuitive to me as well, as the code is saying "get me all of the ancestors , and then just return all of that plus my parent"

heh. yea. I like iterators over recursion. especially while trying to debug. but up to you

hayesr · 2017-02-27T17:45:13Z

The current hash-caching mechanism would be fine, except that the current TreeBuilder implementation wants real objects vs. hashes.

hayesr · 2017-03-09T19:06:40Z

@Fryguy @kbrock Can you take another look at this one? ManageIQ/manageiq-ui-classic#137 is blocked on this. If we don't like it in core, I'll come up with some way of doing it on the UI side.

kbrock · 2017-03-16T13:54:04Z

👍 LGTM (even without those 2 edits)

Fryguy · 2017-03-16T14:12:52Z

app/models/miq_product_feature.rb

+  end
+
+  def self.obj_feature_parent(identifier)
+    obj_features[identifier.to_s] && obj_features[identifier.to_s][:parent]


This pattern is simplified with fetch_path

obj_feature.fetch_path(identifier.to_s, :parent)

Fryguy · 2017-03-16T14:14:01Z

app/models/miq_product_feature.rb

+  end
+
+  def self.obj_feature_children(identifier)
+    obj_features[identifier.to_s] && obj_features[identifier.to_s][:children].select { |c| !c.hidden? }


This pattern is simplified with fetch_path and using reject over select { !

obj_features.fetch_path(:identifier.to_s, :children).try(:reject, &:hidden?)

Fryguy · 2017-03-16T14:15:36Z

app/models/miq_product_feature.rb

+  end
+
+  def self.obj_feature_all_children(identifier)
+    obj_features[identifier.to_s][:children] if obj_features[identifier.to_s]


This pattern is simplified with fetch_path

obj_feature.fetch_path(identifier.to_s, :children)

Fryguy · 2017-03-16T14:16:45Z

app/models/miq_product_feature.rb

+    ancestors = []
+
+    if feature
+      prnt = obj_features[feature[:parent]][:feature]


Please avoid abbreviations. Prefer parent over prnt

The reason for this was to avoid collision with Ancestry's parent method.

it won't collide as it will be an instance variable, but even so, then I would prefer something like parent_feature over prnt

Fryguy · 2017-03-16T14:34:43Z

spec/models/miq_product_feature_spec.rb

+      expect(MiqProductFeature.obj_feature_parent("f4")).to eq("f3")
+    end
+
+    it "finds the ancestors of a feature" do


Can you add a test to handle the edge case of passing an invalid feature name? (which is what the if feature is meant to handle)

Fryguy · 2017-03-16T14:35:32Z

@hayesr Sorry about not reviewing for a while...I got distracted and never got back to this one :/

hayesr · 2017-03-16T16:30:52Z

@Fryguy No worries! I'll address these and ping you.

This speeds up RBAC features tree significantly

hayesr · 2017-03-16T20:46:22Z

@Fryguy ready for another look.

Fryguy · 2017-03-16T20:55:30Z

@hayesr Did you see my follow up comment above? #14037 (comment)

hayesr · 2017-03-16T20:56:31Z

@Fryguy Drat. Missed that sorry.

miq-bot · 2017-03-16T21:02:52Z

Checked commits hayesr/manageiq@b7f6668~...617b56b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

hayesr · 2017-03-16T22:02:17Z

@Fryguy Updated now

hayesr mentioned this pull request Feb 23, 2017

Convert RBAC Features Tree to use TreeBuilder (2) ManageIQ/manageiq-ui-classic#137

Merged

3 tasks

miq-bot added the wip label Feb 23, 2017

hayesr force-pushed the feature_obj_cache branch 2 times, most recently from daf4b97 to d9ba43b Compare February 23, 2017 19:20

hayesr changed the title ~~[WIP] Add a cache for full Feature objects~~ Add a cache for full Feature objects Feb 23, 2017

miq-bot removed the wip label Feb 23, 2017

chessbyte assigned Fryguy Feb 23, 2017

chessbyte requested a review from kbrock February 23, 2017 22:26

Fryguy reviewed Feb 24, 2017

View reviewed changes

Fryguy added performance core/rbac labels Feb 24, 2017

hayesr force-pushed the feature_obj_cache branch from 5987936 to cddeeba Compare February 24, 2017 19:18

kbrock reviewed Feb 27, 2017

View reviewed changes

hayesr force-pushed the feature_obj_cache branch from cddeeba to 0e4ef8c Compare March 9, 2017 18:59

Fryguy reviewed Mar 16, 2017

View reviewed changes

hayesr force-pushed the feature_obj_cache branch from 0e4ef8c to 728352d Compare March 16, 2017 20:33

Add a cache for full objects

b7f6668

This speeds up RBAC features tree significantly

hayesr force-pushed the feature_obj_cache branch from 728352d to b7f6668 Compare March 16, 2017 20:46

Use recursion instead of until loop

617b56b

Fryguy merged commit 53e4995 into ManageIQ:master Mar 22, 2017

Fryguy added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 22, 2017

Add a cache for full Feature objects #14037

Add a cache for full Feature objects #14037

Conversation

hayesr commented Feb 23, 2017

hayesr commented Feb 23, 2017

chessbyte commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Feb 24, 2017 • edited Loading

Fryguy commented Feb 24, 2017

kbrock commented Feb 25, 2017

kbrock commented Feb 25, 2017

chrisarcand commented Feb 27, 2017

kbrock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayesr commented Feb 27, 2017

hayesr commented Mar 9, 2017 • edited Loading

kbrock commented Mar 16, 2017

Choose a reason for hiding this comment

Fryguy Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Mar 16, 2017

hayesr commented Mar 16, 2017

hayesr commented Mar 16, 2017

Fryguy commented Mar 16, 2017

hayesr commented Mar 16, 2017

miq-bot commented Mar 16, 2017

hayesr commented Mar 16, 2017

kbrock commented Feb 24, 2017 •

edited

Loading

kbrock left a comment •

edited

Loading

kbrock Mar 16, 2017 •

edited

Loading

hayesr commented Mar 9, 2017 •

edited

Loading

Fryguy Mar 16, 2017 •

edited

Loading