Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Page tables load very slow in 4.2 #6404

Closed
timkinali opened this issue Apr 18, 2024 · 28 comments · Fixed by #6583
Closed

Page tables load very slow in 4.2 #6404

timkinali opened this issue Apr 18, 2024 · 28 comments · Fixed by #6583
Assignees
Labels
type: performance ⚡️ Is about performance; improves performance

Comments

@timkinali
Copy link

Description

After updating to 4.2, some page sections with layout: table load extremely slow. 100 pages ~10 seconds, 30 pages ~6 seconds. The issue goes away when changing layout to list, cardlets or cards. Confirmed by other users too.

In the forum it was suggested by @rasteiner that the toggle field preview fix in this commit 75eb89c can be the cause:
"It seems to introduce a new instantiation (new Form($model)) which is run for every row in the table."

Your setup

Kirby Version
Kirby 4.2

Additional context
More details in the forum post https://forum.getkirby.com/t/page-sections-load-slow-kirby-4-2-issue/31373/3

@tobimori
Copy link
Contributor

interesting! noticed the same with dreamform submissions since they use the table layout.

@distantnative
Copy link
Member

Tricky one... Indeed before 4.2 the rows weren't processed by the Form class. But that produced wrong values, no defaults etc. – we'll have to see if/how we can make it more performant while using the Form class.

@distantnative distantnative added the type: performance ⚡️ Is about performance; improves performance label Apr 19, 2024
@rasteiner
Copy link
Contributor

PS: I haven't like bisected it or anything, mine was just guess by looking at what changed with table layouts between 4.1 and 4.2.
Maybe the problem's somewhere else.

Anyway if it really were the new Form($model)...
10s/100items = 0.1s/item
That's more than expected for a simple "new Form" (even if that triggers loading the blueprint), so I guess there is some O(n²) recursion going on somewhere.

@distantnative
Copy link
Member

I haven't looked closer at it either yet. But I'd also think this is the main think that changed regarding table values.

@rasteiner
Copy link
Contributor

rasteiner commented Apr 28, 2024

Looked at this again, but it's actually not so easy to reproduce (like: I personally didn't get it to reproduce).

I've created a test setup with the worst things I could imagine, but a pages section with layout: table and limit: 100 still loads in ~70ms on my rather slow test server (php 8.2 in debian docker image on ubuntu) and 4.2.0.

here's the blueprints I've used:
Parent:

title: Test many children
sections:
  pages:
    layout: table
    limit: 100
    columns:
      title: true
      toggle: true

Child:

title: Child

sections:
  files:
    type: files
    layout: cards
  pages:
    type: pages
    layout: table
    parent: page.parent
    limit: 100
  fields:
    type: fields
    fields:
      toggle:
        label: Checkbox
        type: toggle
      info:
        text: the website has {{ site.index.count() }} pages

Could you share the blueprints of the parent and child pages? Maybe also some information on the server where this problem shows?

@gbdesign2023
Copy link

Could you share the blueprints of the parent and child pages?

In my case, ALL children and grandchildren are read out. Of course, I cannot post any blueprints of this. I have posted the blueprint with the table in the forum:
https://forum.getkirby.com/t/page-sections-load-slow-kirby-4-2-issue/31373/10

It feels like the display takes twice as long on the iPhone as on the desktop PC.

@georgobermayrsaf
Copy link

+1 form my side. Have the same issue with pages tables sections. Its not related the the columns I display. The load is way slower in table layout than in one of the other layouts. In my current setups loads take more than 30s for such a section, vs 70ms for a non-table section.

@rasteiner
Copy link
Contributor

rasteiner commented May 8, 2024

+1 form my side. Have the same issue with pages tables sections. Its not related the the columns I display. The load is way slower in table layout than in one of the other layouts. In my current setups loads take more than 30s for such a section, vs 70ms for a non-table section.

could you share the blueprints of both the pages section as well as the page blueprint of the pages being displayed in it?

@timkinali
Copy link
Author

Here's a video... You can see tables loading slow while lists and cards load fast. The table pagination is also slow.
https://www.dropbox.com/scl/fi/lkrtspfguygmiv6ytv9jh/Sk-rminspelning-2024-05-08-kl.-13.30.00.mov?rlkey=k6grulvztglaj9qyuqg2u0b83&st=25dkctjh&dl=0

I also disabled all plugins with no change.

@distantnative
Copy link
Member

@timkinali Could you please still share your blueprints? That would help reproducing and finding the issue (and hopefully a solution).

@georgobermayrsaf
Copy link

could you share the blueprints of both the pages section as well as the page blueprint of the pages being displayed in it?

The section blueprint is rather simple:

label: Subpages
type: pages
templates:
  - storytelling
  - product
  - website.academy.entrypoint
  - website.events.entrypoint
  - website.journal.entrypoint
  - website.jobs.entrypoint
  - website.news.entrypoint
  - website.pressreleases.entrypoint
  - website.webinars.entrypoint
  - loginarea.entrypoint
  - search.results
layout: table
search: true
columns:
  template:
    label: Template
    value: '{{ page.intendedTemplate }}'
    type: tags
  path:
    label: Path
    value: '{{ page.websiteId }}'
  lastUpdate:
    label: Last Update
    value: '{{ page.modified("d/m/Y H:i") }}'
query: page.index(true)
sortBy: title asc
image:
  query: page.teaser.image.toFile
  ratio: 1/1
  cover: true

But the issue is the same with just:

label: Subpages
type: pages
layout: table

I'm afraid I can't share the page blueprint. The setup is too complex with too many extended fields, block field sets and so on. There are all kinds of field types in the blueprint.

@Akasiel
Copy link

Akasiel commented May 15, 2024

I have the same issue. On my local machine it takes 1.6m to load a page table section and in a test environment 51.92s.

@distantnative
Copy link
Member

@Akasiel Any chance you can share the full blueprints?

@georgobermayrsaf
Copy link

I did a bit more digging today. I still can't provide a blueprint, but it seems the main performance issue are blocks fields with custom block types. In my setup I have a lot of them, some of the custom block types even have nested blocks with again custom block types in it. Once I remove the blocks field from the blueprint loading is very fast as in older versions. But if I add one custom block type after another loading gradually gets worse.

@georgobermayrsaf
Copy link

BTW: The move pages dialog seems to have similar issues. Loading of a subpage tree with a lot of subpages (lets say 100) takes very long. From what I gathered in the code, here also the blueprint is completely evaluated. I was also able to pin it down to blocks fields in the blueprint that cause these performance issues.

@afbora
Copy link
Member

afbora commented May 22, 2024

I had an idea. Let me explain with an example:

Imagine there are 1000 note pages under the notes page. When loading the section, the Form class is re-instantiated for each note page. Within this class the page blueprint (note.yml) is loaded. This is done for all 1000 pages. The main reason it is slow in my opinion is that it loads the blueprint 1000 times. If we cache the page blueprint somewhere (in static variable or something like that), only 1 blueprint will be loaded for 1000 pages. This means a very serious performance increase.

What do you think? @getkirby/backend

@timkinali
Copy link
Author

@timkinali Could you please still share your blueprints? That would help reproducing and finding the issue (and hopefully a solution).

@distantnative Sorry for not replying. Like others my blueprints are a bit complex with extends and so forth. I'm happy to share my whole blueprint folder -- or even the entire site -- but not publically. I can however send it to you (or someone else in the Kirby team). Let me know how to go about it!

@distantnative
Copy link
Member

@timkinali if you could send them to support@getkirby.com that would be appreciated. Thanks!

@georgobermayrsaf
Copy link

Is there any news on this one? I just checked with Kirby 4.3, but the issue remains the issue. This is basically blocking some of our projects from updating Kirby.

@distantnative
Copy link
Member

@georgobermayrsaf and others:

Could someone please try if this gets resolved for you by:

  1. Commenting out this line https://github.com/getkirby/kirby/blob/main/config/sections/mixins/layout.php#L137
  2. Changing this part https://github.com/getkirby/kirby/blob/main/config/sections/mixins/layout.php#L143-L148 to
default => $item[$columnName] ?? $model->content()->get($column['id'] ?? $columnName)->value()

This will likely bring back #6249 for now as the current implementation is the proper fix for that. But with the way our blueprint classes greedily resolve everything, it's likely what caused the big performance impact for you. So if this original issue does not affect your setup, dialing it back would be helpful for you.

If we can confirm that this creates relieve for the performance issue, we will likely add a (temporary) blueprint option that disables the proper value fix in favor of keeping the performance for those heavy sites in check.

@gbdesign2023
Copy link

@distantnative
I have also noticed this behaviour:
On site.yml I have a tab with a list of all pages: site.index(true).sortBy('modified', 'desc')
In my case, there are about 150 pages. 50 pages are displayed in the pagination.
If I want to delete an image on a children page, for example, it takes several seconds until the drop-down menu is displayed, although the page has no direct connection to site.yml. If I change the display from table to lists when listing all pages, the speed is as fast as usual.
Conclusion: layout: table on site.yml also reduces the speed of children pages.

@distantnative
Copy link
Member

@gbdesign2023 I don't really know how to understand your response. Did you try out the fix I proposed and does it improve your pages sections with table layout?

@georgobermayr
Copy link

@georgobermayrsaf and others:

Could someone please try if this gets resolved for you by:

  1. Commenting out this line https://github.com/getkirby/kirby/blob/main/config/sections/mixins/layout.php#L137
  2. Changing this part https://github.com/getkirby/kirby/blob/main/config/sections/mixins/layout.php#L143-L148 to
default => $item[$columnName] ?? $model->content()->get($column['id'] ?? $columnName)->value()

This will likely bring back #6249 for now as the current implementation is the proper fix for that. But with the way our blueprint classes greedily resolve everything, it's likely what caused the big performance impact for you. So if this original issue does not affect your setup, dialing it back would be helpful for you.

If we can confirm that this creates relieve for the performance issue, we will likely add a (temporary) blueprint option that disables the proper value fix in favor of keeping the performance for those heavy sites in check.

Thank you @distantnative! Yes, I can confirm the impact of the change. I tested with the loading of a complex pages section:

  • Kirby 4.1: around 500ms
  • Kirby 4.3: 51,2s (!)
  • Kirby 4.3 with your changes: back to around 500ms

@distantnative
Copy link
Member

@georgobermayr we will add it as a blueprint option to 4.4 that reverts it to the old buggy but performant behavior. It's not ideal but a perfect solution involves huge refactorings and won't be ready that soon. So we will take a pragmatic approach here, so everyone can choose the lesser evil for their specific project (which I think will allow a large number of projects run problem-free).

@timkinali
Copy link
Author

timkinali commented Jul 24, 2024

Yep! I can also confirm this fixed the issue @distantnative! (Kirby 4.2). Thank you.

Would you mind detailing a bit on what to look out for bringing back #6249, not sure I understand from the issue description. Is it only toggle field previews or does it affect other field previews?

@distantnative
Copy link
Member

Of course. We have the following dilemma:

  • Pre 4.2 we would pass the raw values from the content file to the table preview components. Which caused problems with the toggle field as it would receive a string not a bool. While it wasn't reported, I would assume this behavior would also clash with other previews, e.g. pages.
  • In v4.2 we fixed this by running the content values through the Form class and field definitions to retrieve the actual values (and context data) needed. This is the right solution for the problem that the field previews would not receive the correct data. However, it can lead to huge performance impacts as reported in this issue. This is because our field and blueprint classes resolve everything very greedily, unfortunately. So setups that somewhere in their blueprints work with the site index or just any performance-heavy thing really get tolled as likely for every row the blueprints are newly resolved etc.

Both have their downside. The ideal solution is to refactor our blueprint classes and make them more lazy and performant. This is needed and import but unfortunately also nothing we will solve in the next months easily (also because we're currently working on another hard but important refactoring).

Left with this unfortunate situation, I have been thinking that there are two ways to ease the situation:

  • Introduce a new blueprint option for the table layout, e.g. raw values: true that reverts the behavior to pre-4.2. Meaning that some previews won't work then in your setup, but at least your setup doesn't load in over 30 seconds. Probably a better choice for many and in general I think this is a good intermediary solution as it allows each developer to pick which is the lesser evil for their specific project. Some might not have any performance issues right now and prefer all previews to work correctly, others didn't have any preview issues before 4.2 and would just love to get their performance back.
  • There will be a crowd left for whom both choices are bad. For those we really have to advocate to don't use site index or similar as willingly in blueprints. These are super heavy operations. Without the ideal solution on the horizon, working on caching collections etc. will be their best choice for now.

@timkinali
Copy link
Author

Ah makes sense, it resolves the full blueprint and not only the columns that are actually defined for the table. I see how this have an impact for some of my sections.

For me your suggested intermediary solution is a good compromise. 👍

@distantnative distantnative linked a pull request Jul 29, 2024 that will close this issue
5 tasks
@distantnative
Copy link
Member

We have added the rawvalues: true option for v4.4.

Closing this in favor of the clean summary issue which we will keep around until fully fixing the performance issue: #6594

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
type: performance ⚡️ Is about performance; improves performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants