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.

Sign up for GitHub

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

Jump to bottom

Precache rollup values #22594

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Precache rollup values #22594

merged 4 commits into from
Jul 5, 2023

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jun 29, 2023

reference:

For Metrics rollup, we fetch the container_image from each of the container objects.
This change preloads the parent container_image value into the containers so we avoid the N+1.

TODO: I need to followup to find the test case to ensure I'm fixing the issue at hand

before

container_image = ContainerImage.first
containers = container_image.vim_performance_state(timestamp).containers
containers.map(&:container_image) # N+1

after

container_image = ContainerImage.first
containers = container_image.vim_performance_state(timestamp).containers
containers.map(&:container_image) # ==> array of container_image object

@kbrock kbrock mentioned this pull request Jun 29, 2023
Closed
15 tasks
Fryguy
Fryguy reviewed Jun 29, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 29, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 29, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
@kbrock
Copy link
Member Author

kbrock commented Jun 29, 2023

update:

  • fixed comments
  • fixed new preload call (I had swapped the arguments)

Fryguy
Fryguy reviewed Jun 29, 2023
View reviewed changes
spec/lib/miq_preloader_spec.rb Outdated
expect { vms.map(&:ext_management_system) }.to make_database_queries

vms.each(&:reload)
expect { MiqPreloader.preload_reverse(ems, :vms, vms) }.not_to make_database_queries
Copy link
Member

Choose a reason for hiding this comment

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

Oh but now I'm seeing :ems in the test - this is confusing. I think I know an interface that would make this less complicated, but I need to discuss.

@kbrock
Copy link
Member Author

kbrock commented Jun 30, 2023

Moving the guts to #22589

Keeping this PR for comments

Fryguy
Fryguy reviewed Jun 30, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 30, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 30, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 30, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 30, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 30, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
Fryguy
Fryguy reviewed Jun 30, 2023
View reviewed changes
lib/miq_preloader.rb Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Jun 30, 2023

@kbrock Now that we decided to switch to using the original reload and we got rid of the "extra" method, is there anything substantive in this PR anymore? Seems it's just yard docs and specs. If that't the case can you update the OP and perhaps the labels?

@kbrock kbrock force-pushed the precache_rollup_values branch 2 times, most recently from 0794dc3 to 22dedb6 Compare July 3, 2023 18:04
@kbrock
Copy link
Member Author

kbrock commented Jul 3, 2023

update:

  • s/to_not/not_to (specs only)
  • changed VimPerformanceStates to use scopes
  • changed VimPerformanceState#capture (spec only)
  • VimPerformanceState#vim_performance_state_association preloads forward/reverse association

update:

  • fix white space issues

This was referenced Jul 3, 2023
Merged
Merged
@kbrock kbrock force-pushed the precache_rollup_values branch 2 times, most recently from 16b542b to 477a5ca Compare July 5, 2023 15:39
@Fryguy Fryguy self-assigned this Jul 5, 2023
@kbrock
specs for MiqPreloader
04ee08a 
added docs around using a scope to populate the relation
This is only used in one place by rollups.

I had thought an array would work, but it does not.
@kbrock
Cache vim_performance_state relations
f7f3bdb 
we were having trouble that the reverse relation (assoc.parent) was not being cached
Ended up deciding caching the forward and the reverse are the best
@kbrock
Add support for scopes to MiqPreloader#preload
ac9f6ca 
We want to use preloaded data to populate a record's associations
Rails currently accepts a scope for preloading, but it runs the scope for every input record

We are using this to load VimPerformanceState values for a single record
We also want the list of records after it is done.

Before
======

```ruby
ems = ExtManagementSystem.take(5).to_a
vms = Vm.where(:ems => ems.map(&:id)).load

MiqPreloader.preload(ems, :vms, vms)      # => 5 queries
MiqPreloader.preload(ems, :vms, vms.to_a) # error
```

After
=====

```ruby
ems = ExtManagementSystem.take(5).to_a
vms = Vm.where(:ems => ems.map(&:id)).load

MiqPreloader.preload(ems, :vms, vms)      # => 5 queries
MiqPreloader.preload(ems, :vms, vms.to_a) # error
```
@miq-bot
Copy link
Member

miq-bot commented Jul 5, 2023

Checked commits kbrock/manageiq@4f40c52~...ac9f6ca with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
7 files checked, 1 offense detected

lib/extensions/ar_preloader.rb

@kbrock
Copy link
Member Author

kbrock commented Jul 5, 2023

update:

@Fryguy Fryguy merged commit f28d107 into ManageIQ:master Jul 5, 2023
@kbrock kbrock deleted the precache_rollup_values branch July 6, 2023 20:11
@kbrock kbrock mentioned this pull request Jul 7, 2023
Merged
1 task
kbrock added a commit to kbrock/manageiq that referenced this pull request Jul 7, 2023
@kbrock
Revert "Merge pull request ManageIQ#22594 from kbrock/precache_rollup…
bd6431c 
…_values"

This reverts commit f28d107, reversing
changes made to 4b5fe60.
@Fryguy
Copy link
Member

Fryguy commented Jul 25, 2023

Backported to petrosian in commit 3987dea.

commit 3987deafd36d4aae5064660e544a0af328faaf33
Author: Jason Frey <fryguy9@gmail.com>
Date:   Wed Jul 5 15:55:18 2023 -0400

    Merge pull request #22594 from kbrock/precache_rollup_values
    
    Precache rollup values
    
    (cherry picked from commit f28d10779b15316e27578175c07145a3b35469bf)

Fryguy added a commit that referenced this pull request Jul 25, 2023
@Fryguy
Merge pull request #22594 from kbrock/precache_rollup_values
3987dea 
Precache rollup values

(cherry picked from commit f28d107)
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 9, 2023
@kbrock
Run a single query for pre-loading VimPerformanceStates
2a74a16 
This bug was introduced by ManageIQ#22579

I misread this code and thought it was fetching all records a single time.
Turns out it is running this query N+1 times.

So adding the extra resource_id values (and a large count at that) to each query,
means we still run N+1, we fetch the same number of records, but our query
is much larger

After
=====

We are running a single query to fetch all VimPerformanceStates

Depends upon preloader patch: ManageIQ#22594
@kbrock kbrock mentioned this pull request Aug 9, 2023
Merged
kbrock added a commit to kbrock/manageiq that referenced this pull request Aug 10, 2023
@kbrock
Run a single query for pre-loading VimPerformanceStates
4888bd6 
This bug was introduced by ManageIQ#22579

I misread this code and thought it was fetching all records a single time.
Turns out it is running this query N+1 times.

So adding the extra resource_id values (and a large count at that) to each query,
means we still run N+1, we fetch the same number of records, but our query
is much larger

After
=====

We are running a single query to fetch all VimPerformanceStates

Details
=======

One fix would have been to add a `load`, but that depends upon
our preloader monkey patch: ManageIQ#22594

The number of queries are the same, the index usage is the same, the number
of rows returned is the same for unchanged, load, and not specifying the resource

But the PR in question does double the size of the query string.
Which is a problem when rolling up a container image from a bunch of containers.

I opted to change the code rather than rollback because I liked the extra comments
around the block of code.

In the future, we want to consolidate the caching of vim_performance_states
as we currently have 2 methods of caching those values:
- preload that association
- store in an @ variable.


Numbers
=======

|       ms |query |  qry ms |     rows |` comments`
|      ---:|  ---:|     ---:|      ---:|  ---
|  4,808.1 |   19 | 2,736.4 |    1,334 |` 340-2023-06-22 20:00:00Z-long-1#before`
|    130.2 |    1 |   130.2 |          |`. SELECT vim_performance_states.* -- [30991]`
|      ---:|  ---:|     ---:|      ---:|  ---
|  4,431.2 |   19 | 2,500.3 |    1,334 |` 340-2023-06-22 20:00:00Z-short-1#after`
|     82.6 |    1 |    82.5 |          |`. SELECT vim_performance_states.* -- [15036]`
|      ---:|  ---:|     ---:|      ---:|  ---
|  4,659.0 |   19 | 2,690.4 |    1,334 |` 340-2023-06-22 20:00:00Z-load-1#alt`
|    107.9 |    1 |   107.9 |          |`. SELECT vim_performance_states.* -- [15036]`

alt being to use load instead of removing resource from the query
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy approved these changes

@agrare agrare Awaiting requested review from agrare agrare is a code owner

Assignees

@jrafanie jrafanie

@Fryguy Fryguy

Labels
performance petrosian/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kbrock @Fryguy @miq-bot @jrafanie