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

[TwigComponent] Centralize, reuse and warm-up component properties metadata #2211

Open
wants to merge 6 commits into
base: 2.x
Choose a base branch
from

Conversation

smnandre
Copy link
Collaborator

Currently if we have 50 times the same component in a page, 50 times we use reflection to analyse component class properties and methods.

This PR centralize this task in a dedicated (internal) service and add a cachewarmer to pre-compute metadata during app build.

Significant performance gains here too
(i won't do charts for every PR but be sure i'm gonna make some before/after once i'm "done" with all this.... in Vienna 👼 )

First version to replicate behaviour + some code fixes

In an IRL app, the number of class-based components is not something "big".
So this could/should be computed once during warm up.

Next steps:
- adapt DI
- introduce cache + cachewarmer
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Sep 26, 2024
Copy link
Collaborator

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Looks pretty cool! Thanks @smnandre

src/TwigComponent/src/ComponentRenderer.php Outdated Show resolved Hide resolved
src/TwigComponent/src/ComponentRenderer.php Outdated Show resolved Hide resolved
smnandre and others added 4 commits September 26, 2024 20:35
Co-authored-by: Hugo Alliaume <[email protected]>
Co-authored-by: Hugo Alliaume <[email protected]>
Co-authored-by: Hugo Alliaume <[email protected]>
@onEXHovia
Copy link
Contributor

As an alternative, or even as a follow-up, I think it makes sense to replace/improve class ComponentMetadata. We could get all possible meta information PostMount/PreMount/ExposeInTemplate hooks etc, not just about properties.

Structure may look like this:
ComponentMetadata - store metadata
ComponentMetadataFactory - provider metadata with methods getMetadataFor/hasMetadataFor. Factory methods return ComponentMetadata instance.
ComponentCacheWarmer - cache warmer call factory ComponentMetadataFactory

Copy link
Collaborator

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Are we agree, this is happening only in prod ? I don't see anything prod related

@smnandre
Copy link
Collaborator Author

Hey @onEXHovia ! You read my mind

This is indeed my next move, and i started to extract the props metadata from template too :)

@smnandre
Copy link
Collaborator Author

Are we agree, this is happening only in prod ?

@WebMamba nothing "prod-related"

For all the environments, during a request, there is no need to compute metadata twice, so the values are "kept".

Also, if you use cache:warm all the components metadata are cached in the ssytem cache.

What's maybe missing here is something to invalidate the cache locally if you change a file ?

Do you have encountered any trouble ?

@Kocal
Copy link
Collaborator

Kocal commented Sep 29, 2024

image

A small rebase before merging? :) 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Reviewed Has been reviewed by a maintainer TwigComponent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants