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

Active Storage eager loading: support more methods #42981

Merged
merged 2 commits into from
May 16, 2022
Merged

Active Storage eager loading: support more methods #42981

merged 2 commits into from
May 16, 2022

Conversation

ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Aug 9, 2021 •
edited

As noted at #40842 (comment), the current implementation works great with the processed method, but doesn't work with other methods such as key. This PR fixes that, by adding the missing includes statements.

cc @collimarco

@ghiculescu ghiculescu force-pushed the as-eager-more-methods-3 branch 2 times, most recently from 8f87cf1 to 0fce237 Compare August 9, 2021 21:20
@collimarco
Copy link

Wow, thank you so much!

I was struggling to find a solution... I couldn't find where in the call chain the blob was loaded again. Now from your work I guess that the record.image association had no idea about the blob association and thus loaded it again, while if we access to the blob directly from the VariantWithRecord, we still have access to it.

I have quickly tested this in our application and everything seems to work, perfect.

@ghiculescu
Copy link
Member Author

ghiculescu commented Aug 9, 2021 •
edited

Hmmm, this has broken some tests. I think this solution might have other issues. Will investigate but probably not before tomorrow.

@ghiculescu ghiculescu force-pushed the as-eager-more-methods-3 branch 5 times, most recently from e1fcedc to b6c8aa1 Compare August 9, 2021 22:53
@ghiculescu ghiculescu marked this pull request as draft August 9, 2021 22:55
@collimarco
Copy link

There's one thing that I don't understand in the new code: (let @transformed=false) calling variant key is like calling blob.key... Isn't that the key of the original file? In this case we need the key of the variant, not the key of the original uploaded file.

@collimarco
Copy link

As in my previous comment, I confirm that we need to call record&.image.key and not record&.blob.key, because they are different (one is the key of the variant, the other the key of the original file).

The record&.image.class is ActiveStorage::Attached::One... however I can't find the definition of #key in that class. Any idea where it is defined? (I have even tried record&.image.method(:key).source_location, but it is empty)

@collimarco
Copy link

collimarco commented Aug 11, 2021 •
edited

@georgeclaghorn I see that you originally wrote the code for VariantWithRecord. Any hint on how to generate the variant key without hitting the database again? (considering that the associations Attachment -> Blob -> Variants are already eager loaded using includes)

@ghiculescu
Copy link
Member Author

ghiculescu commented Aug 11, 2021 •
edited

I think I'll have some more time tomorrow to work on this

@ghiculescu ghiculescu force-pushed the as-eager-more-methods-3 branch 2 times, most recently from 99b06af to 4739afe Compare August 13, 2021 16:05
@ghiculescu
Copy link
Member Author

Simplified my approach, the solution is just to add the missing includes.

@ghiculescu ghiculescu marked this pull request as ready for review August 13, 2021 16:06
@ghiculescu ghiculescu force-pushed the as-eager-more-methods-3 branch 2 times, most recently from 8dab38c to b7eb4bd Compare August 13, 2021 16:08
@ghiculescu ghiculescu added the ready PRs ready to merge label Aug 13, 2021
@ghiculescu ghiculescu mentioned this pull request Aug 13, 2021
Merged
@collimarco
Copy link

Thanks for this improvement!

So in order to get the item.image.variant({}).key we need to:

  1. Load the Attachment
  2. Load the Blob (original file)
  3. Load the Variant
  4. Load another Attachment (image of the Variant)
  5. Load another Blob (the Variant image file)

That's really deep nesting... I expected to be able to compute the Variant key directly from the Variant, I didn't expected that the Variant doesn't represent the file, but it needs to be attached again to the actual file (Blob).

I don't understand this choice, but I guess there are good reasons for that. Also this is not a decision made now by this pull request, but it was made when the Variant was stored in the database.

In any case I have tested this PR in production (together with #40842) and it seems to work great! Despite the deep nesting, I've seen a 10x improvement on pages that have hundreds of images. Thanks again!

@collimarco
Copy link

Can someone merge this? It's really useful for performance

@ghiculescu
Copy link
Member Author

@pixeltrix could I bother you for a review here? It's just fixing a bug in #40842

@daveallie
Copy link

@ghiculescu while you're here, can you please apply the same fixes to the has_one_attached version of ActiveStorage attachments: https://github.com/rails/rails/blob/b7eb4bdfea1128340b701d79dc5190d193d645be/activestorage/lib/active_storage/attached/model.rb#L73

They suffer from the same N+1 issue when doing bulk queries, and would benefit from the same deep includes call.

@ghiculescu
Active Storage eager loading: support more methods
675264a 
As noted at rails#40842 (comment), the current implementation works great with the `processed` method, but doesn't work with other methods such as `key`. This PR fixes that.

Why is this fix necessary? Currently when you call `VariantWithRecord#key`, `key` gets called on `VariantWithRecord` -> `Attached::One` -> `Attachment` -> `Blob`. This results in n+1 calls to load the `Attachment`. But for this purpose we shouldn't need to load it, as the `VariantWithRecord` already knows about it relevant `Blob`. So this PR changes the method to call directly to that blob.
@ghiculescu
Copy link
Member Author

@rafaelfranca this fixes a bug that was found in 7.0 alpha, could it please be included in the next RC?

@rails-bot
Copy link

rails-bot bot commented Mar 8, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Mar 8, 2022
@collimarco
Copy link

Keep it open. Please someone should merge this...

@rails-bot rails-bot bot removed the stale label Mar 8, 2022
jorgemanrubia
jorgemanrubia approved these changes May 14, 2022
View reviewed changes
Copy link
Contributor

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Good one @ghiculescu 👍. We'll get this one merged (cc @jeremy).

@jeremy jeremy added this to the 7.1.0 milestone May 16, 2022
@jeremy
Merge branch 'main' into as-eager-more-methods-3
f24500b 
* main: (746 commits)
  Address QueryCacheTest#test_query_cache_does_not_allow_sql_key_mutation failure
  Fixes ActiveStorage proxy downloads of files over 5mb in S3-like storage services
  Fixes development Action Mailbox new mail form
  Squash commits
  Include the unexpected class in InvalidParameterKey message
  Support unbounded time ranges for PostgreSQL
  Fix CHANGELOG alignment [ci-skip]
  Add ability to ignore tables by regexp for SQL schema dumps
  Improve `rails s` error message when no server could be found.
  Fix MySQL warning when creating Active Record's test databases
  Add `--js` and --skip-javascript` options to `rails new`
  Fix parsing operator classes for index columns in PostgreSQL
  Fix rails test command to handle leading dot slash
  Document that url_for can take classes
  Don't change the encoding of frozen parameters
  Update working_with_javascript_in_rails.md
  Avoid query from calculations on contradictory relation
  Fix MemoryStore#write(name, val, unless_exist: true) with expired entry
  Provide pattern matching for ActiveModel
  Stop autoclosing of PRs
  ...
@jeremy jeremy merged commit fe8d41e into rails:main May 16, 2022
@ghiculescu ghiculescu deleted the as-eager-more-methods-3 branch May 16, 2022 20:08
@ghiculescu
Copy link
Member Author

Thanks!

@collimarco
Copy link

Great, thanks 🎉

@ghiculescu ghiculescu mentioned this pull request Dec 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgemanrubia jorgemanrubia jorgemanrubia approved these changes

Assignees
No one assigned
Labels
activestorage ready PRs ready to merge
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ghiculescu @collimarco @daveallie @jorgemanrubia @jeremy