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

TreeBuilderReportReport reduce queries #1253

Merged
merged 2 commits into from
May 5, 2017

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented May 4, 2017

related to #1228

Overview

The page /reports/explorer uses many builders, but this PR is only focused on one.

TreeBuilderReportReports is simple:

  • The root nodes are high-level report categories (from the menu)
  • The child nodes are low-level report categories (from the menu)
  • The grandchild nodes are MiqReport records (names from the menu).

Before

  • The number of grandchildren is counted from the database.
  • Grandchildren are looked up individually.

After

  • There are no counts for grandchild since the values come from the menu.
  • Grandchildren are looked up using a single query per child (the parent of the grandchildren).

Numbers

ms query query ms rows comments
313.5 51 111.1 468 #1228 applied
258.7 23 86.6 440 #1288 and this applied
17.5% 55% 22.1% 6% diff

Note: While #1228 and this PR are independent, the numbers seemed more honest if they included that patch in place.

Expanded Reports Numbers

ms query query ms rows comments
2,895.3 803 1,891.5 823 #1228 applied
1,408.6 444 787.3 823 #1228 and this applied
51.3% 45% 58.4% 0 diff

These are the numbers when all report categories are expanded. Since report records are brought back, the count(*) savings are lost, but the N+1 really shines through.

Please treat these numbers with a grain of salt, it is unlikely that a user will ever expand all levels of the tree and then refresh the screen.

High Git Change count

Since the @rpt_menu lookups confused me, I renamed and added many variables

Also, it seemed simpler to group the category lookups into one method (i.e.: get_tree_roots) and the report node lookups into another (e.g.: get_custom_kids).

I can reduce the lines changed significantly if you prefer that route.

Added feature

@dclarizio mentioned that we want to remove report names from the menu. I added a switch (i.e.: REPORTS_IN_TREE) to document how to do this.

This prevents the report lookup so the worst case numbers go from 803 queries down to the 23.

Bug

Tree builders work in a few ways:

  1. level by level - e.g.: root returns [node1, node2], level1 returns [node11, node12], level2...
  2. complete tree - e.g. root returns {node1 => {node11 => {}, node12 => {}}, node2 => {}}
  3. partial tree - e.g.: root returns {node1 => [node11, node12], node2 => []}, and level2 returns [node111, node 1112]

It turns out that the 3rd use case did not work properly. I made a change in tree_builder.rb to address this issue.

@skateman
Copy link
Member

skateman commented May 4, 2017

Looks good to me, but @ZitaNemeckova's review should be the deciding one 😉

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

LGTM

:icon => "pficon #{@grp_title == r[0] ? 'pficon-folder-close-blue' : 'pficon-folder-close'}",
:tip => r[0]
)
@rpt_menu.each_with_index.each_with_object({}) do |(r, node_id), a|
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still work with count_only?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought x_get_tree_roots only called with count_only = true in rare instances where trees are build from trees.

I added a fix.
Thanks

@dclarizio
Copy link

@kbrock any cc issues you can address? Thx, Dan

We fetch folders and report for the menu

Bring back all folders nodes as a hash (get_tree_roots)
Bring back report nodes (get_custom_kids)

Added a switch not display reports in the tree.
Fixed bug in tree_builder that would force all records to be downloaded
at once
@kbrock
Copy link
Member Author

kbrock commented May 4, 2017

@ZitaNemeckova Could you also check the Bug section above?

I feel most people don't understand the 3 ways tree builders work and hope to share that information with everyone creating and fixing trees.

That list is my primary means of speeding up the left-hand navigation tree.

@kbrock kbrock force-pushed the tree_reports_r_r branch from 2a85d90 to 1c50bb5 Compare May 4, 2017 20:38
@miq-bot
Copy link
Member

miq-bot commented May 4, 2017

Checked commits kbrock/manageiq-ui-classic@d84f42d~...1c50bb5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

Copy link
Contributor

@ZitaNemeckova ZitaNemeckova left a comment

Choose a reason for hiding this comment

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

Tested in UI. LGTM 👍

@himdel himdel added this to the Sprint 60 Ending May 8, 2017 milestone May 5, 2017
@himdel
Copy link
Contributor

himdel commented May 5, 2017

So.. looks like everything works, I also tested a bunch of random trees to catch the treebuilder changes .. 👍

@himdel himdel merged commit e3eb865 into ManageIQ:master May 5, 2017
@kbrock kbrock deleted the tree_reports_r_r branch May 5, 2017 21:47
@kbrock
Copy link
Member Author

kbrock commented May 5, 2017

Thanks @himdel

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