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

Improve dropdown rendering speed #10838

Closed
wants to merge 9 commits into from
Closed

Conversation

lekoala
Copy link
Contributor

@lekoala lekoala commented Jun 26, 2023

Fixes #10174

Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

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

Failing tests are related, please review.

Apart from that, I'm not personally sure about this change. I get that preparing the list in PHP might be faster, but with that (at a stretch) we can scrape the whole templating system and do everything in PHP.

@lekoala
Copy link
Contributor Author

lekoala commented Jun 26, 2023

@michalkleiner yes i'm not sure why the tests are failing i need to review this and see what are the tests doing

the template system is fine for simple things, but not great for loops. rendering large list of items (eg: a dropdown with country names) is quite slow. i do think there is an issue with the templating system itself, but that's clearly out of scope :-)

@michalkleiner
Copy link
Contributor

Saw the comment here so I got more context now. Let's see what other @silverstripe/core-team thinks about the topic.

@lekoala
Copy link
Contributor Author

lekoala commented Jun 26, 2023

@michalkleiner tests all green ;-)

Co-authored-by: Michal Kleiner <mk@011.nz>
@GuySartorelli
Copy link
Member

I don't like this change. If there's a problem with the templating system that makes it too slow for this and similar scenarios, then resolving that problem is the fix. This feels like a project-level workaround.

I especially agree with Michal's comment: "I get that preparing the list in PHP might be faster, but with that (at a stretch) we can scrape the whole templating system and do everything in PHP."
If we start pulling template logic into PHP here, where does that end? This doesn't feel like the correct approach to the problem. I'm going to close this PR on that basis.

@lekoala
Copy link
Contributor Author

lekoala commented Jun 27, 2023

@GuySartorelli i agree with you: i don't like this change. but when a page takes 2 times more to render because of a couple of dropdowns, that's not something that can be ignored either :-)

ideally, i think profiling should be done on the templating system and see where are the bottlenecks

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.

Rendering DropdownField can be really slow for many instances
3 participants