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

Eagerload at the controller or job layer #2313

Merged
merged 7 commits into from Apr 27, 2022

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Apr 22, 2022

What is this pull request for?

(Extracted from #2309)

This moves preloading of stuff to the Job layer rather than do it at the controller layer. Two advantages: We don't preload anything we don't need, and the element repository of the page version can be used in other contexts as well (I had a case where I wanted to load multiple pages and preload their ingredients, and using the previous element repository would break that).

Notable changes (remove if none)

We now also preload ingredients. We do that explicitly at the controller level, which means we can use the PageVersion#element_repository in contexts like the following:

@page.children.where(page_layout: "blog_post).includes(public_version: :elements).each { |p| p.public_version.elements } 

Checklist

@mamhoff mamhoff force-pushed the do-not-eagerload-in-page-version branch 7 times, most recently from b251b6f to 4ea6672 Compare April 26, 2022 13:42
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

This is really great. I have a question about the publish job

app/jobs/alchemy/publish_page_job.rb Outdated Show resolved Hide resolved
@mamhoff mamhoff force-pushed the do-not-eagerload-in-page-version branch 2 times, most recently from b8dd989 to 77c3659 Compare April 27, 2022 09:16
@mamhoff mamhoff changed the title Do not eagerload in page version Eagerload at the controller or job layer Apr 27, 2022
It's not great to eager load stuff at the model level. In case a
previous user of the data has preloaded already, we're now preloading
twice. Since preloading is expensive, we should avoid that and commit to
only loading from model or service objects when there's no controller we
can hook into.
@mamhoff mamhoff force-pushed the do-not-eagerload-in-page-version branch from 77c3659 to ac8555f Compare April 27, 2022 09:17
We need to preload a bit of stuff to publish a page, notably all the
elements, ingredients, and related objects of its draft version. We
probaly don´t need to load much of the public version, or any other
versions, as those are not relevant to the operation.
Note that we're only preloading the public version here.
Here, we need to preload the draft version because that's what we look
at.
This spec mocked many things unnecessarily.
Passing an page "object" here through `GlobalID` and then reloading will
lead to double page loads.
@mamhoff mamhoff force-pushed the do-not-eagerload-in-page-version branch 2 times, most recently from 1ac0d43 to 32d2b92 Compare April 27, 2022 10:08
@tvdeyen tvdeyen force-pushed the do-not-eagerload-in-page-version branch 2 times, most recently from 429f126 to 7e5268c Compare April 27, 2022 10:22
This helps keep the duplication down.
@tvdeyen tvdeyen force-pushed the do-not-eagerload-in-page-version branch from 7e5268c to dbcfd93 Compare April 27, 2022 10:33
@tvdeyen tvdeyen enabled auto-merge April 27, 2022 10:36
@tvdeyen tvdeyen merged commit 8e6200d into AlchemyCMS:main Apr 27, 2022
tvdeyen added a commit that referenced this pull request May 30, 2022
@wuservices
Copy link
Contributor

@mamhoff We just upgraded to 6.0.12 from 4.0 and noticed a significant performance regression and it appears to be due to this change. We verified by temporarily rolling back to 6.0.5 and the previous response times returned.

After we upgraded, we immediately noticed a lot more time spent in pg/postgres and that transactions were taking 2.5x as long overall at p50 in Datadog. Most of the requests we observe are just a small Alchemy page that's used as a health check. Therefore, the partials for this page are typically cached.

It seems like even if we don't need to get any data, it's eagerly being loaded. There were originally some eager loading optimizations added in Alchemy 4.4 (#1674), but @tvdeyen mentioned there that eager loading was not added non-API HTML requests to avoid unnecessary work for cached pages.

It appears that the performance slowdown I observed is related to that consideration where we now perform extra queries even if they wouldn't have been needed. However, on a cold cache, it does seem to improve performance. Maybe even adding nested elements could further reduce queries, but that's a separate question.

Is it intended that we'll now have slower performance on cached page fragment hits? Is there any way to eager load, but only if the page isn't cached?

@tvdeyen
Copy link
Member

tvdeyen commented May 10, 2023

@wuservices hey, thanks for bringing this to our attention. Would you be so kind and create a fresh Alchemy app where we can reproduce the problem? That would really help us to fix this, because - no - it is not intended to have slower performance 😏

cc @mamhoff

@tvdeyen
Copy link
Member

tvdeyen commented May 10, 2023

@wuservices @mamhoff I created an issue to further discuss the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants