-
-
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
Skip folded deeper levels when rendering page tree #1324
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Thank you, Pascal.
I tried this locally with my "Seed 1000 Alchemy Pages" gist and did not saw any noticeable improvements, but this might be because of running this locally with sqlite and in dev mode.
As I did not found any regressions, 👍
Could you add a simple model spec for the newly introduced scope? Thanks
Previously the page tree was rendered in full even if pages are folded. Consider the following tree: even if level 1 was folded, level 2 and 3 would get rendered and sent to the client. - root |+ level 1 |- level 2 |- level 3 This change does two things: 1. load all user-folded pages for the current user in advance, reducing the number of queries to 1 2. skips all pages that are under a folded page The result is: - root |+ level 1 Together with 5a4c0d3 this speeds up the page tree rendering from 20 seconds to ~100ms when only 10 folded pages are shown (Alchemy::Page.count == 4000).
f7eae6e
to
ac564d7
Compare
Thanks for the gist! Yes, this certainly has an influence. I have no time to dig deeper into this, but it seems Postgres has certain problems with this part of the code or maybe it's my 3.5 branch (although including 5a4c0d3). I tried it with roughly 3k pages (5 levels with 5 sub pages each) and it outperforms my Postgres testsetup. However: both are faster with this patch:
Also notice the difference in size. In the 3.5k pages example this goes down from 600kb to 4KB. I added a model spec. The test for inheritance doesn't seem to be necessary any more. I copied it from the old implementation, but I'm not brave enough to remove it and added a test to verify it doesn't fail for non-AR user classes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the specs and additional benchmarks.
Danke Pascal und lieben Gruß |
Great work! What's the timeline for the 4.1 release? |
This change is a performance improvement and should not affect the behavior of the CMS.
Previously the page tree was rendered in full even if pages are folded.
Consider the following tree: even if level 1 was folded, level 2 and 3
would get rendered and sent to the client.
This change does two things:
the number of queries to 1
The result is:
Together with 5a4c0d3 this speeds up the page tree rendering from 20
seconds to ~100ms when only 10 folded pages are shown (for
Alchemy::Page.count == 4000
). The number of queries per page tree is also constant now.Feedback is welcome! 👂