Use OpenStruct over MiqHashStruct

[NickLaMuro]

This is almost a straight 1 for 1 conversion, and performs much better than our implementation: MiqHashStruct.

---

Regarding the [WIP] portion:

Update: This section is no longer relevant as I have found the solution to the issue (tests caught what was supposed to be happening). Keeping the below for posterity, but see the last commit for updates to the rational here.

The only part that doesn't work (but also doesn't make any sense that it ever did), was the portion of this code that was commented out:

- if tz[1].to_i_with_method == val.to_i_with_method
-   # Save [value, description] for timezones array
-   init_values[field_name] = [val, tz[0]]
- end
+ # if tz[1].to_i_with_method == val.to_i_with_method
+ #   # Save [value, description] for timezones array
+ #   init_values[field_name] = [val, tz[0]]
+ # end

tz in the above always seems to be a MiqHashStruct (in the previous commit), and when tz[1] is called, it hit's method_missing with :[] as the method argument. This of course will almost never have a match in the underlying @hash in the MiqHashStruct, but will not blow up at least.

When it is an OpenStruct, this fails with an error since 1 can't be converted to a symbol properly. I am uncertain if this code is ever activated, so for now, I have just commented it out to observe the advantages to using OpenStruct over MiqHashStruct.

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

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

Total allocated: 2069091751 bytes (23226536 objects)   |   Total allocated: 1592827666 bytes (11222271 objects)
                                                       |   
allocated objects by gem                               |   allocated objects by gem
-----------------------------------                    |   -----------------------------------
   9665227  pending  <<<<<<<<<<                        |      3513258  manageiq/lib
   5561668  manageiq/app  <<<<<<<<<<                   |      3074174  manageiq/app  <<<<<<<<<<
   3513258  manageiq/lib                               |      2512007  activerecord-5.0.7
   2512007  activerecord-5.0.7                         |       861263  activesupport-5.0.7
    861270  activesupport-5.0.7                        |       418737  ancestry-2.2.2
    418737  ancestry-2.2.2                             |       278793  activemodel-5.0.7
    278793  activemodel-5.0.7                          |       244083  ruby-2.3.3/lib
    178419  ruby-2.3.3/lib                             |       165577  arel-7.1.4
    165577  arel-7.1.4                                 |        82799  pending  <<<<<<<<<<
     52875  manageiq-providers-vmware-0be2f13a0dc9     |        52875  manageiq-providers-vmware-0be2f13a0dc9
     14424  fast_gettext-1.2.0                         |        14424  fast_gettext-1.2.0
       ...                                             |          ...

Another analysis that included other fixes in addition to this, but gives an alternative solution as well (though it becomes quite obvious why this was chosen), can be found here.

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

• Partially addresses https://bugzilla.redhat.com/show_bug.cgi?id=1569626
• Full collection of proposed changes: git compare
• More in depth benchmarks and different strategies to solve this: gist
• Document keeping track of overall improvements for this effort: gist
• Related PRs:
  • Prevents N+1 SQL queries in miq_request_workflow.rb #17354
    • Extracted PRs: Add MiqPreloader.polymorphic_preload_for_child_classes #17457, Use MiqPreloader.polymorphic_preload_for_child_classes in RelationshipMixin#fulltree_arranged #17460, Use MiqPreloader/Relationship improvements in MiqRequestWorkflow #17461
  • Memoize _log.prefix calls #17355
  • Use nested hashes instead of string keys #17357
  • Refactor get_ems_folders to create less strings #17358
  • 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

[NickLaMuro]

@miq-bot assign @Fryguy
@miq-bot add_label performance

@Fryguy I was informed that you might have a opinion against this change, so looping you in. Feel free to pass this off to @gmcculloug if you don't really care (though, wouldn't mind both of your opinions on this if possible).

[syncrou]

@NickLaMuro - This looks good to me, though with your comment in the description I'll defer to @Fryguy in regards to his concerns.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[gmcculloug]

I am good with this one. Will leave it to @Fryguy who has more knowledge on the internals of MiqHashStruct.

[miq-bot]

Checked commit NickLaMuro@58a6a6b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 2 offenses detected

app/models/miq_provision_virt_workflow.rb

• ❗ - Line 1049, Col 5 - Style/MultilineIfModifier - Favor a normal if-statement over a modifier clause in a multiline statement.
• ❗ - Line 1052, Col 5 - Style/MultilineIfModifier - Favor a normal if-statement over a modifier clause in a multiline statement.

[NickLaMuro]

Note: This change and the one below it are here because the previous version of the spec is grabbing the internal variable inside the MiqHashStruct, and using that to test the resulting values. @hash is the internal hash variable in MiqHashStruct, and @table is the equivalent in OpenStruct.

I personally think it make more sense to avoid the instance_variable_get all together, and just do a send(:attr) instead, but I didn't want to change the tests that I hardly understand in the first place. They were added recent, however, in these two PRs:

• Add support for sysprep customization templates #17293
• Add customization template specs for cloud-init #17477

[Fryguy]

You can just call result.first.to_h if all you want is the hash. That works equivalently in MiqHashStruct and OpenStruct.

[NickLaMuro]

Good point. To be fair, I didn't write this test in the first place, so I was just trying to keep the spec as close to as it was originally. I can change it to the .to_h suggestion if you prefer.

[Fryguy]

The purpose of MiqHashStruct is specifically because of memory/performance issues with OpenStruct. It was created as a drop-in replacement for OpenStruct, with mostly a similar interface (at least the parts we used of it). In this particular PR's case it might be ok to use it. If you're doing performance analysis you should take into account what MiqHashStruct is trying to solve, which I'm not sure your numbers are doing or not.

The reason for it (at least in the past...I haven't checked recently) is that when you initialize an OpenStruct it creates (def) methods on that specific instance for each key in the incoming hash. Since it's per instance, if you have 1000 OpenStructs, those methods are created on 1000 separate objects. MiqHashStruct doesn't do that, intentionally, instead using method_missing. The purpose of the method def in OpenStruct is for fast reads and writes, but if you read/write infrequently, or perhaps only read/write a small amount out of many attributes, the memory and time overhead of creating those extra, unused methods outweighs a dynamic method_missing lookup.

In other words, MiqHashStruct is more efficient in high-instantiation / low-read/write cases (i.e. create 1000 OpenStructs, and only read 1 property from each), whereas OpenStruct is more efficient is low-instantiation / high-read cases (i.e. a cached global object created on boot where most attributes are read 1000s of times).

Here is a gist of the performance concerns: https://gist.github.com/Fryguy/ac9adffcc48d343fb30d1c0373819357

MiqHashStruct.new-large
                        274.544k (±23.2%) i/s -      1.310M in   5.013370s
OpenStruct.new-large      4.372k (± 4.0%) i/s -     21.850k in   5.005783s
MiqHashStruct.new-small
                          1.279M (± 4.7%) i/s -      6.478M in   5.075022s
OpenStruct.new-small    423.267k (± 9.4%) i/s -      2.095M in   5.001133s

MiqHashStruct high-read
                          1.907k (± 7.3%) i/s -      9.506k in   5.013554s
OpenStruct high-read      8.319k (± 3.4%) i/s -     41.769k in   5.026756s
MiqHashStruct low-read
                          1.807M (± 6.1%) i/s -      9.084M in   5.047288s
 OpenStruct low_read      6.422M (± 6.4%) i/s -     32.128M in   5.025546s

[NickLaMuro]

MiqHashStruct is more efficient in high-instantiation / low-read/write cases

@Fryguy Well, unfortunately that isn't the case. This whole process is basically taking ActiveRecord objects, converting them to "pick your struct of choice", and then converting those "structs" into XML. There are also some cases where those "structs" are also used to traverse the tree to then instantiate more ActiveRecord objects, to then be turned into "structs", prior to the XML bit.

So this is incredibly read heavy on the attribute end, and exactly the rational that MiqHashStruct wasn't built for. If you look at the gist provided for this PR (and all related ones):

https://gist.github.com/NickLaMuro/36f9f394a5b3cecc4aaf33d3e84dfb27#cpu-usage

you will see that the biggest 'time suck' following (garbage collection), is MiqHashStruct#method_missing, which is precisely the spot where MiqHashStruct falls flat.

---

So not sure the point you are making here. This clearly, based on the data for this particular use case that I have provided, proves that MiqHashStruct is the wrong tool for the job.

Frankly, I don't think either struct option makes sense if at the end of the day they are just getting feed into a XML blob, and we can just remove the intermediate object all together. But that is a much bigger and risker change than I was willing to do that the time for something here that seems like a pretty quick win.

[Fryguy]

@NickLaMuro Can you explain what your benchmarks show? You highlight pending and manageiq/app, but I don't know what those mean. I also don't understand how MiqHashStruct -> OpenStruct can have such a dramatic change, considering they don't do very much. In other words, it's possible you could get the same benefit with some minor tweak to MiqHashStruct as well.

[Fryguy]

If you look at the gist provided for this PR (and all related ones):

I'll be honest, there is just way too much information, organized in a way that doesn't work for me (I find the Links section too complicated for how I process information, as it basically dumps a lot of P.S. after my mind has already context switched away from whatever paragraph each individual thing should be related to), so I didn't see that you had linked a gist at all.

Separately, in the gist I see the time went from 15s to 1s, but here the time goes from 15s to 14s. I don't see how they can be so drastically different. The gist talks about SQL statements, so is this just one part of a set of changes just to keep it isolated?

So not sure the point you are making here.
=========
Will leave it to @Fryguy who has more knowledge on the internals of MiqHashStruct.

Just educating on what MiqHashStruct's purpose for existence is, since there seems to be a misunderstanding, and it's what was asked of me. I really don't have time to read through everything, but I felt that giving the information and saying "pick the right tool for the job" was sufficient. As I said, "in this particular PR's case it might be ok to use it", but I couldn't tell if this fell into the high-read or low-read case.

This clearly, based on the data for this particular use case that I have provided, proves that MiqHashStruct is the wrong tool for the job.

Yup, that's what I was looking for, and your analysis seems to prove that out, so I am 👍

Frankly, I don't think either struct option makes sense if at the end of the day they are just getting feed into a XML blob, and we can just remove the intermediate object all together.

💯 Agree. A lot of the older code uses OpenStructs and such just to get dot-access, when it's really not necessary. Much better to just access the hashes directly, or if there is a reason for making a struct, then we should make a real-proper struct.

[jrafanie]

This is almost a straight 1 for 1 conversion, and performs much better than our implementation: MiqHashStruct.

Rereading this PR, I see discussion of CPU and memory differences in the OpenStruct vs. MiqHashStruct and yet I only see one or the other. Because we're talking about a specific area and we are open to changing which struct we use for that area depending on the benchmarks, does it make sense showing OpenStruct vs. MiqHashStruct on the same specific pages in terms of time and memory? Or even better, do all the pages have the same entrypoint that is callable from rails console?

I'm afraid we're trading memory for cpu or vice versa for different scenarios (high read/ low read / large struct/small struct) and it's hard to measure which is "better" for the most cases for this code area if we don't test the expected scenarios and show the CPU/memory with each struct.

Note, I don't generally think allocations is a good measure for comparing things. Long lived objects vs. short lived objects have completely different performance characteristics so we can't really compare one allocation to another. If allocations are important, I'd expect us to pay the penalty in memory/CPU time (doing GC)... so maybe we can just watch memory/CPU time since that's really all anyone cares about.

[NickLaMuro]

Can you explain what your benchmarks show? You highlight pending and manageiq/app, but I don't know what those mean.

@Fryguy Sure. This is an excerpt from the memory_profiler output when running profiling the highlighted method in this script.

Specifically, MemoryProfiler is just a wrapper around ObjectSpace.trace_object_allocations, which keeps track of where the object allocations occurred in the codebase, and presents it nicely (I am sure you know this, but mentioning just in case).

What you are seeing highlighted is there is a significant reduction in objects being allocated specifically in the manageiq-gems-pending portion of the, and specifically in the MiqHashStruct#method_missing section which is hit heavily in this use case. Another key thing of note is that nothing else really goes up, meaning OpenStruct isn't making things worse while fixing it in other areas (though, that could be assumed by just looking at the total allocations as well).

---

I also don't understand how MiqHashStruct -> OpenStruct can have such a dramatic change, considering they don't do very much. In other words, it's possible you could get the same benefit with some minor tweak to MiqHashStruct as well.

I actually tried this first, believe it or not. Since this is boarding to months old now, though, I forget where I put the diff of the changes I made around MiqHashStruct, but I can highlight a few things:

• Moved "=" to a frozen constant
• Used avoided String#[] to get last character (object allocation) and used String#substring or String#char_at
• Avoiding the .to_s and .to_sym calls when possible
• Plus there is all of the conditional logic in there as well.

(update: found the diff ... and guess were it was... the links section...)

The overarching theme is that since #method_missing is used for both getters and setters, and there is no real better solution that I could come up with than what OpenStruct does, which is to lazy apply getters and setters on first hit to either a @hash[key] or @hash[key] = val respectively. Obviously doing that is the quickest read/write mechanic, and the logic we have in #method_missing makes that much slower than the ideal.

At the end of the day, I found that I was either going to "rewrite OpenStruct's ideas" to make MiqHashStruct faster, or I could just use OpenStruct.

---

Just educating on what MiqHashStruct's purpose for existence is, since there seems to be a misunderstanding, and it's what was asked of me.

Sorry, my reaction may have been a bit harsh. I read your first comment as "we should keep using MiqHashStruct, and here is why instead of "this is why MiqHashStruct exists in the first place".

---

I'll be honest, there is just way too much information...

Separately, in the gist I see the time went from 15s to 1s, but here the time goes from 15s to 14s. I don't see how they can be so drastically different. The gist talks about SQL statements, so is this just one part of a set of changes just to keep it isolated?

Yes it is long, but this is trying to help rationalize why these 15+ PRs have a benefit together, but also the the individual impact when compared to all of the changes combined. I tried to omit as much information as possible in this PR (and the rest) specifically because of your tl; dr attitude from the past (which, again, I get but it gets frustrating repeating myself and having to write long follow ups as a result).

And, as mentioned in the description at the end of the benchmarks section:

" 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. "

[NickLaMuro]

Rereading this PR, I see discussion of CPU and memory differences in the OpenStruct vs. MiqHashStruct and yet I only see one or the other.

@jrafanie I tried to keep the data across all of the related PRs in a format that you could infer CPU, Memory, and SQL performance improvements/stagnation with the minimal amount of data (again, trying to solve for the tl; dr crowd).

With this PR specifically, I assumed that a "savings in CPU" could be inferred by a drop in 1s taken to run this scenario with only the change of s/MiqHashStruct/OpenStruct, and that we used less memory by there being a VERY LARGE overall drop in object allocations.

Again, more detailed info for the "verbose crowd" can be found in the gist

Because we're talking about a specific area and we are open to changing which struct we use for that area depending on the benchmarks, does it make sense showing OpenStruct vs. MiqHashStruct on the same specific pages in terms of time and memory? Or even better, do all the pages have the same entrypoint that is callable from rails console?

I do all of this. The gist I linked to has a script that I have been running to validate these changes before and after. It is runnable by doing a:

$ bin/rails r sample_profile_script.rb

I have omitted the profiling code in to make it easier to grok, but you can profile it with whatever you want.

I'm afraid we're trading memory for cpu or vice versa for different scenarios (high read/ low read / large struct/small struct) and it's hard to measure which is "better" for the most cases for this code area if we don't test the expected scenarios and show the CPU/memory with each struct.

I assume you are referring to other controller actions or portions of the codebase that call to this model specifically not being tested. To that, I have no clue what that might be, or where to start.

Note, I don't generally think allocations is a good measure for comparing things.

Disagree. While this doesn't give you a good ballpark figure of how much memory is actually used, and the 24mil objects allocated could just be ` byte strings, it is superior in being incredibly consistent and reliable between runs. Having a consistently reproducible metrics/telemetry allows the changes I make be much more scientific and the resulting reports much more concrete.

Also, when paired with the CPU metrics that I provide in the gist, it is much more obvious how much of an impact all of these extra objects are having on CPU performance as well.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[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!

[Fryguy]

@NickLaMuro Should this be reopened? I think it just fell by the wayside, and I had already 👍ed it here: #17359 (comment)

@jrafanie Since Nick's numbers showed a benefit, and I didn't really see a direct 👎 from you, are you ok with it?

[NickLaMuro]

@Fryguy So I am actually working on something in the same area, and found that messing with MiqHashStruct/OpenStruct didn't have to much of effect, and based on your previous comment, might actually be a problem if we switch based on the scale that I am working with (around 120k templates loaded for a AWS EMS).

The PRs in question:

• Performance improvements to MiqProvisionVirtWorkflow#allowed_templates #18353
• [WIP] Add cache columns to operating_systems table manageiq-schema#322
• [POC][WIP] Make use of platform column to further increase the speed of #allowed_templates #18354

Specifically, the last two PRs (required together), show that we can basically get down to about 4 seconds to run MiqProvisionVirtWorkflow#allowed_templates in the particular dataset, and half of that is just running the SQL to fetch that 120k records. A big amount of my savings came in the form of skipping active record calls and just returning hashes.

Something to consider, but I didn't originally plan on bringing this back from the dead since it seems like we can get some wins in other ways.

[jrafanie]

Yeah, I was ok with this change but in light of what @NickLaMuro said, it makes sense to go after the more valuable changes than this one. We can always resurrect it if we can pinpoint it as a performance problem later.

[djberg96]

@NickLaMuro I vote to re-open as part of ManageIQ/manageiq-gems-pending#231

[NickLaMuro]

Might be unmergable, so might be a bit of work to do.

Will look into doing this in a bit.