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

Memoize image_path helper in build_tags_tree #10181

Conversation

NickLaMuro
Copy link
Member

Purpose or Intent

Improves the load time of the /miq_request/show/:id route for databases with a large number of Tags (over 10k).

Overview

Making the call to ActionController::Base.helpers.image_path is expensive and is called N times for the total number of tags (parents and children) iterated over in build_tags_tree. Since this value never changes for each parent and child and in the iteration, saving these before entering the loop saves significant amount of time and resources when N grows large.

Metrics

Here is an example showing a before and after with a DB that includes 15k Tags:

Before

Started GET "/miq_request/show/:id"...
Completed 200 OK in 13204ms (Views: 919.9ms | ActiveRecord: 0.0ms)

ms bytes objects queries query (ms) rows
11,811.7 N/A 38,383,207 78 905.3 33,593

After

Started GET "/miq_request/show/:id"...
Completed 200 OK in 3819ms (Views: 1380.0ms | ActiveRecord: 0.0ms)

ms bytes objects queries query (ms) rows
1,974.4 N/A 2,146,710 78 843.9 33,593

Making the call to `ActionController::Base.helpers.image_path` is
expensive, and since this value never changes for each parent and child
and in the iteration, saving these before entering the loop saves
significant amount of time when N grows large.

Here is an example showing a before and after with a DB that includes
15k Tags:

Before
------

Started GET "/miq_request/show/:id"...
Completed 200 OK in 13204ms (Views: 919.9ms | ActiveRecord: 0.0ms)

|       ms |   bytes |    objects | queries | query (ms) |   rows |
|     ---: |    ---: |       ---: |    ---: |       ---: |   ---: |
| 11,811.7 |     N/A | 38,383,207 |      78 |      905.3 | 33,593 |

After
-----

Started GET "/miq_request/show/:id"...
Completed 200 OK in 3819ms (Views: 1380.0ms | ActiveRecord: 0.0ms)

|      ms |   bytes |   objects | queries | query (ms) |   rows |
|    ---: |    ---: |      ---: |    ---: |       ---: |   ---: |
| 1,974.4 |     N/A | 2,146,710 |      78 |      843.9 | 33,593 |
@miq-bot
Copy link
Member

miq-bot commented Aug 2, 2016

Checked commit NickLaMuro@8bf1511 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 👍

@chessbyte
Copy link
Member

@NickLaMuro @dmetzger57 @dclarizio should this be backported to Darga?

@chessbyte chessbyte assigned chessbyte and unassigned dclarizio Aug 2, 2016
@chessbyte chessbyte merged commit 7c2e4ee into ManageIQ:master Aug 2, 2016
@chessbyte chessbyte added this to the Sprint 44 Ending Aug 1, 2016 milestone Aug 2, 2016
@NickLaMuro
Copy link
Member Author

@chessbyte Sorry about the forgetting labels 😞

As far as darga is concerned, I think it is a simple enough fix with enough speed/memory improvements that it should be an easy and worthwhile backport. It might not hurt having someone from @dclarizio 's team give it a once over though just to make sure I didn't miss something, since build_tags_tree is used in a few spots in that helper, and is included in the application controller.

@dmetzger57
Copy link
Contributor

Yes 👍

Sent from my iPhone

On Aug 1, 2016, at 10:21 PM, Oleg Barenboim notifications@github.com
wrote:

@NickLaMuro https://github.com/NickLaMuro @dmetzger57
https://github.com/dmetzger57 @dclarizio https://github.com/dclarizio
should this be backported to Darga?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10181 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALtdey1fz7hD4Fjpokuiy6YHGwZ2vgatks5qbqm5gaJpZM4JaIbv
.

chessbyte added a commit that referenced this pull request Aug 23, 2016
…ld_tags_tree

Memoize image_path helper in build_tags_tree
(cherry picked from commit 7c2e4ee)
@chessbyte
Copy link
Member

Darga backport details:

commit d009e0777abf7d3d6c64dc3db2675735e4b952fe
Author: Oleg Barenboim <chessbyte@gmail.com>
Date:   Mon Aug 1 22:21:48 2016 -0400

    Merge pull request #10181 from NickLaMuro/memoize_image_paths_for_build_tags_tree

    Memoize image_path helper in build_tags_tree
    (cherry picked from commit 7c2e4ee4c2173b654499a19ed91a439e1c6d9777)

@JPrause
Copy link
Member

JPrause commented Sep 12, 2016

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.

None yet

7 participants