Skip to content

Better detection of standard preload #160

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 2, 2024

This ends up being an optimization.

Before

First off, includes(:association) has includes_values = [:association], or the uses clause stores an array.

When the framework uses the ActiveRecord::Associations::Preloader, the associations are passed in. If the associations go in via a single association (e.g.: :books), then our Preloader#call patch detects this and calls super right away.

But since includes(:association) stores includes_values as an array (e.g.: [:association]) or uses as an array, we often call the Preloader.call with the array of associations.

This second scenario, creates an extra preloader, then group_by the records creating a second array. It is this extra preloading, grouping, and array that is avoided.

In our tests, this does not happen often, but when running miq tests, this case seemed to happen often.

After

Preload#call will also detect if the association is an array with a single symbol. The extra processing is avoided.

@kbrock kbrock added the bug Something isn't working label Aug 2, 2024
@kbrock kbrock force-pushed the preload_array_assoc branch from 4ca1bc3 to bf2af28 Compare August 2, 2024 19:47
@kbrock
Copy link
Member Author

kbrock commented Aug 2, 2024

update:

  • fixed trailing whitespace (rubocop)
  • swapped order of the operands (rubocop)

@kbrock kbrock force-pushed the preload_array_assoc branch from bf2af28 to 17cb22d Compare August 6, 2024 18:29
@kbrock kbrock force-pushed the preload_array_assoc branch from 17cb22d to a37b1df Compare August 6, 2024 19:52
@miq-bot miq-bot added the stale label Nov 11, 2024
@miq-bot
Copy link
Member

miq-bot commented Nov 11, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Feb 17, 2025

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@kbrock kbrock force-pushed the preload_array_assoc branch from a37b1df to 173049d Compare June 20, 2025 15:32
@Fryguy Fryguy removed the stale label Jun 20, 2025
@kbrock kbrock force-pushed the preload_array_assoc branch from 173049d to 87358d8 Compare June 24, 2025 15:08
@jrafanie
Copy link
Member

Kicking so it tests with rails 7.2

@jrafanie jrafanie closed this Jun 24, 2025
@jrafanie jrafanie reopened this Jun 24, 2025
@kbrock kbrock force-pushed the preload_array_assoc branch from 87358d8 to 5cd9dee Compare June 25, 2025 18:26
This ends up being an optimization.
But when preloading `[:association]`, it runs a separate preloader on `:association`.
This skips the interim step
@kbrock kbrock force-pushed the preload_array_assoc branch from 5cd9dee to 36bbc38 Compare June 25, 2025 20:11
@miq-bot
Copy link
Member

miq-bot commented Jun 25, 2025

Checked commit kbrock@36bbc38 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.62.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM

@jrafanie jrafanie merged commit d6a3c90 into ManageIQ:master Jun 25, 2025
6 checks passed
@kbrock kbrock deleted the preload_array_assoc branch June 26, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants