-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
Performance regression in 6.0 due to unnecessarily eager loading #2468
Comments
I created a fresh app in https://github.com/wuservices/alchemy-debug-performance on 6.0.5, then added a 6.0.6 branch to compare performance. See README there for methodology. This confirmed in isolation what we noticed in production. After upgrading to 6.0.6, the eager loading will still fetch data via SQL even if the fragment cache is warmed up. In our production app, here's what upgrading to 6.0.12 looked like where you can see the latency is over 2x on average (most of these are cached), and the percentage of overall transaction time spent in the database increases. You can then see we dip back down at the end, which is where I reverted to 6.0.5 for testing. Below are logs from the fresh test instance where I compare how many queries are executed with cold/empty vs warm caches on 6.0.5 vs 6.0.6. 6.0.5 empty cache (19 queries)
6.0.5 warm cache (11 queries)
6.0.6 empty cache (22 queries)
6.0.6 warm cache (22 queries)
The best solution would be if we didn't have to run the eager loading unnecessarily, but that I'm not sure if that's possible to know at the right time since the templates aren't hit yet, and they may or may not have fragment caching. I suspect this was the original concern in #1674. As is, there's still a question of whether this is better than not eager loading at all. One might argue that ideally, the Alchemy origin server shouldn't be getting these kinds of cached requests anyways. However, if the app can still respond efficiently when caches are cleared, that will mitigate any slowdown when intermediate (e.g. CDN) caches are cleared. |
Pages make use of fragment caching and we cannot know which element is cached. Therefore we cannot know what to eager load. Closes AlchemyCMS#2468 This reverts commit d556574.
Pages make use of fragment caching and we cannot know which element is cached. Therefore we cannot know what to eager load. Refs AlchemyCMS#2468 This reverts commit d556574.
Pages make use of fragment caching and we cannot know which element is cached. Therefore we cannot know what to eager load. Closes AlchemyCMS#2468 This partially reverts commit 0aa84d0.
@wuservices thanks for the great write up and all this infos. This really helped to understand the issue. We agree that this is an regression that needs to be reverted. I created a PR that only reverts the change in question at #2474 Would you mind trying that one and verify it removes the performance regression? Thanks again for bringing this one up. |
Pages make use of fragment caching and we cannot know which element is cached. Therefore we cannot know what to eager load. Closes AlchemyCMS#2468 This partially reverts commit 0aa84d0.
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?
Originally posted by @wuservices in #2313 (comment)
The text was updated successfully, but these errors were encountered: