Skip to content

Cache matching node renderers #784

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

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

wouterj
Copy link
Contributor

@wouterj wouterj commented Dec 27, 2023

By caching which node renderer can be used for which node, we can avoid a lot of foreach loops.

From a quick local test (with pcov enabled), this improved run time by 50%. Blackfire diff: https://blackfire.io/profiles/compare/9aa42af9-7692-418c-9d34-f7e010a22f8f/graph

We can optionally do this in a non-BC breaking way by introducing a new cacheable interface with a supportsFqcn() method (similar to Symfony's voter caching). However, I can find no node renderer in guides and Symfony that does something other than instanceof. So I would favor simplicity for the years to come over this BC break.
But I'm happy to discuss rewriting this to a smooth upgrade path if this helps TYPO3.

@jaapio
Copy link
Member

jaapio commented Dec 27, 2023

I would suggest to take the reverse path, do it this way... and if we need a separate renderer for nodes by content we can always introduce this in a different way especially because this is a huge performance improvement. I think right now nobody is using the generic supports method right now.

Thanks for this improvement. Wouter!

Copy link
Contributor

@linawolf linawolf left a comment

Choose a reason for hiding this comment

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

Thanks for this great improvement. The guides are already so fast and still so much opportunity to improve, amazing!

I don't think we have any custom node renderers in TYPO3 so for us it is not breaking.

Great work!

@jaapio jaapio force-pushed the node-renderer-factory-perf branch from 10175c4 to cbf0744 Compare December 27, 2023 21:30
@jaapio jaapio enabled auto-merge December 27, 2023 21:30
@jaapio jaapio merged commit dce1c96 into phpDocumentor:main Dec 27, 2023
@wouterj wouterj deleted the node-renderer-factory-perf branch December 27, 2023 22:14
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.

3 participants