Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nested hashes instead of string keys #17357

Merged

Conversation

NickLaMuro
Copy link
Member

@NickLaMuro NickLaMuro commented Apr 27, 2018

For both instantiating the values, and retrieving the data, using string based keys requires a bunch of new object allocations for something that is simply used for a faster lookup for data already put into the XML structure (I think...), by using a nested hash structure, we can keep the same "lookup" functionality, but not require the extra allocations to do so.

As far as I can tell, the only place this lookup is used is in one method, so this should be okay to change.

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 13753 1961 48395
Before                                                     After
------                                                     -----

Total allocated: 2069091751 bytes (23226536 objects)   |   Total allocated: 1970420998 bytes (21117758 objects)
                                                       |   
allocated objects by gem                               |   allocated objects by gem
-----------------------------------                    |   -----------------------------------
   9665227  pending                                    |      9665227  pending
   5561668  manageiq/app  <<<<<<<<<<                   |      4155069  manageiq/app  <<<<<<<<<<
   3513258  manageiq/lib                               |      3513258  manageiq/lib
   2512007  activerecord-5.0.7                         |      2512007  activerecord-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                          |       178419  ruby-2.3.3/lib
    178419  ruby-2.3.3/lib                             |       165577  arel-7.1.4
    165577  arel-7.1.4                                 |       159091  activesupport-5.0.7  <<<<<<<<<<
     52875  manageiq-providers-vmware-0be2f13a0dc9     |        52875  manageiq-providers-vmware-0be2f13a0dc9
     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

This avoids needing to generate a new string value for each key on
setting of the value and retrieving, and the extra hashes are not a huge
consumer of space.

Of note, the keys are now not strings at all.  Top level will be the
class names, and the child keys will be just the raw IDs.  The
@ems_xml_nodes value is not used anywhere besides the lookup that was
changed in this commit, so the change of this structure should not have
an adverse affect... hopefully...
@NickLaMuro NickLaMuro force-pushed the nested_hashes_for_miq_request_workflow branch from 60aaf40 to ef73d6e Compare April 27, 2018 16:29
@miq-bot
Copy link
Member

miq-bot commented Apr 27, 2018

Checked commit NickLaMuro@ef73d6e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@NickLaMuro
Copy link
Member Author

@miq-bot assign @gmcculloug
@miq-bot add_label performance

Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickLaMuro - This looks good to me.

@kbrock kbrock assigned kbrock and unassigned gmcculloug May 18, 2018
@kbrock kbrock added this to the Sprint 86 Ending May 21, 2018 milestone May 18, 2018
@kbrock kbrock merged commit 5f22e97 into ManageIQ:master May 18, 2018
@NickLaMuro
Copy link
Member Author

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

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

Use nested hashes instead of string keys
(cherry picked from commit 5f22e97)

https://bugzilla.redhat.com/show_bug.cgi?id=1593798
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 831c6435387ec9b5d69355523d3c80df31c0f218
Author: Keenan Brock <[email protected]>
Date:   Fri May 18 12:21:35 2018 -0400

    Merge pull request #17357 from NickLaMuro/nested_hashes_for_miq_request_workflow
    
    Use nested hashes instead of string keys
    (cherry picked from commit 5f22e971f034528138b6be42181ee4ba90461fdb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593798

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

Use nested hashes instead of string keys
(cherry picked from commit 5f22e97)

https://bugzilla.redhat.com/show_bug.cgi?id=1593797
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit de6d40d4967862e0ea4d673fe8efea255b6d4f19
Author: Keenan Brock <[email protected]>
Date:   Fri May 18 12:21:35 2018 -0400

    Merge pull request #17357 from NickLaMuro/nested_hashes_for_miq_request_workflow
    
    Use nested hashes instead of string keys
    (cherry picked from commit 5f22e971f034528138b6be42181ee4ba90461fdb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1593797

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants