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

[EUWE] Do not store ar object for old refresh #14673

Merged

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Apr 6, 2017

A not clean cherry-pick of #13594

Storing AR object around is causing memory bloat. While
doing .reload on AR object is decreasing the memory bloat,
it is adding a processing time, cause .reload does a SQL query
per each object then.

Therefore removing the AR object is decreasing memory required
as well as the processing time.

Implements:
https://www.pivotaltracker.com/story/show/137741549

@miq-bot miq-bot changed the title Do not store ar object for old refresh euwe [EUWE] Do not store ar object for old refresh euwe Apr 6, 2017
@Ladas Ladas changed the title [EUWE] Do not store ar object for old refresh euwe [EUWE] Do not store ar object for old refresh Apr 6, 2017
@Ladas
Copy link
Contributor Author

Ladas commented Apr 10, 2017

@Fryguy could you review also this one? there are stats in the upstream PR #13594

@simaishi
Copy link
Contributor

@Ladas can you resolve the conflict?

@agrare
Copy link
Member

agrare commented Apr 11, 2017

@simaishi is the result of #14542
Take what is in HEAD

<<<<<<< HEAD
    # Lets first index the hashes based on keys, so we can do O(1) lookups
    record_index = records.index_by { |record| build_index_from_record(keys, record) }
    record_class = records.first.class.base_class

    hashes.each do |hash|
      record = record_index[build_index_from_hash(keys, hash, record_class)]
      hash[:id] = record.id
=======
    hashes.each do |h| 
      r = records.detect { |r| keys.all? { |k| r.send(k) == r.class.type_for_attribute(k.to_s).cast(h[k]) } } 
      h[:id] = r.id
>>>>>>> Do not store ActiveRecord object anywhere in saving code.

Storing AR object around is causing memory bloat. While
doing .reload on AR object is decreasing the memory bloat,
it is adding a processing time, cause .reload does a SQL query
per each object then.

Therefore removing the AR object is decreasing memory required
as well as the processing time.
Use :id instead of :_object for vms saving
Use :id instead of :_object for saving of cloud_tenants
Nullify array of the to be 'deleted' objects, this was causing
a reference and the AR objects were not GCed then.
Do not build vm_and_templates through association, rails are
caching all the objects there, making it huge in memory.
Use :id instead of :_object for NetworkManager saving. We can
also delete processing of cross-manager links (
orchestration_stacks, vms, availability_zones, cloud_tenants)
since those will be always AR objects.

Only :device can be both Hash and AR object, so it's handled
appropriatelly.

The ignored cross-manager links processing was needed only
when there were CloudManagers without NetworkManagers, so this
is not backportable to Darga.

 Conflicts:
   app/models/ems_refresh/save_inventory_network.rb
Associate subnets with routers with less queries, 50% of the
DB queries exactly.
@Ladas Ladas force-pushed the do_not_store_ar_object_for_old_refresh_euwe branch from 7628ff9 to 8307f2a Compare April 11, 2017 13:49
@miq-bot
Copy link
Member

miq-bot commented Apr 11, 2017

Checked commits Ladas/manageiq@e651717~...8307f2a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 1 offense detected

app/models/ems_refresh/save_inventory_network.rb

@simaishi simaishi merged commit 1b5829b into ManageIQ:euwe Apr 11, 2017
@simaishi simaishi added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 11, 2017
agrare pushed a commit to agrare/manageiq that referenced this pull request Apr 19, 2017
…or_old_refresh_euwe

[EUWE] Do not store ar object for old refresh
(cherry picked from commit 1b5829b)

https://bugzilla.redhat.com/show_bug.cgi?id=1441202
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.

4 participants