Skip to content

[LiveComponent] Optimize LiveComponentStack::getCurrentLiveComponent() #2821

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
Jun 8, 2025

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Jun 4, 2025

Q A
Bug fix? yes
New feature? no
Docs? no
Issues Fix #...
License MIT

Using Reflection during runtime is costly. Here I suggest using static caching to prevent unnecessary usages of Reflection.

I've re-used reproducer from #2812, but with 50 rows (otherwise I can't create a Blackfire Profile), and it reduced the rendering time by 200ms, https://blackfire.io/profiles/compare/7aa62248-9332-40b3-b6fa-58756cb17232/graph
image

EDIT: see #2821 (comment)

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jun 5, 2025
@norkunas
Copy link
Contributor

norkunas commented Jun 5, 2025

Should this use kernel.reset, or you think it will be a small cache and never releasing memory in worker mode will be imperceptible?

@Kocal
Copy link
Member Author

Kocal commented Jun 5, 2025

Should this use kernel.reset, or you think it will be a small cache and never releasing memory in worker mode will be imperceptible?

Oh, that's a good point, I didn't think about it.

I think we are safe here, given this reproducer:

<?php
$componentsCount = 100;
$cache = [];

echo sprintf('Memory usage before: %d MiB', memory_get_peak_usage(true) / 1024 / 1024 ) . PHP_EOL;

for ($i = 0; $i < $componentsCount; $i++) {
    $cache['\App\Twig\Components\ComponentA' . $i] = $i % 2;
}
echo sprintf('Memory usage after: %d MiB', memory_get_peak_usage(true) / 1024 / 1024 ) . PHP_EOL;
  1. If you have 100 components (something more realistic) or 1000 components, the memory does not change:
Memory usage before: 2 MiB
Memory usage after: 2 MiB
  1. With 10k components, +2 MiB:
Memory usage before: 2 MiB
Memory usage after: 4 MiB
  1. And with 100k components (???), only 13 MiB:
Memory usage before: 2 MiB
Memory usage after: 13 MiB

The memory usage is not an issue.

And [] !== (new \ReflectionClass($classname))->getAttributes(AsLiveComponent::class) will always return the exact result between requests (unless you modify your components files without redeploying your app 😅) so that's a win-win, the runtime cache is persisted during a request and also for next requests (when using FrankenPHP worker's mode, Swoole, etc...)!

@Kocal Kocal changed the title [LiveComponent] Optimize LiveComponentStack::isLiveComponent [LiveComponent] Optimize LiveComponentStack::isLiveComponent() Jun 5, 2025
@Kocal Kocal force-pushed the live-component/perf-isLiveComponent branch from f6ea6cc to b866cdc Compare June 5, 2025 07:18
@Kocal
Copy link
Member Author

Kocal commented Jun 5, 2025

I went for another solution, where we can completely shortcut isLiveComponent() call, by storing the static cache in getCurrentLiveComponent().

It gives better results, https://blackfire.io/profiles/compare/8bbc8e11-fbad-4853-bb08-9cbdcb857e50/graph:
image

@Kocal Kocal force-pushed the live-component/perf-isLiveComponent branch from b866cdc to cc7f4fa Compare June 5, 2025 07:24
@Kocal Kocal changed the title [LiveComponent] Optimize LiveComponentStack::isLiveComponent() [LiveComponent] Optimize LiveComponentStack::getCurrentLiveComponent() Jun 5, 2025
@smnandre
Copy link
Member

smnandre commented Jun 7, 2025

We've been using maps of component names -> stuff in various places for some time now, and they've helped winning major optimizations, while I have not received nor read any downsides.

But more importantly ...

The class is used here, not the name. That means all the anonymous components in a given project count as one.

Finally, I've never heard of a project with a thousand TwigComponent classes for now... and i'm pretty sure there would be a lot of other problems before 😶

@smnandre
Copy link
Member

smnandre commented Jun 7, 2025

TL;DR; maybe let's see if there is some problem here and we will adapt accordingly :)

(i'm talking about the kernel.reset here, not this PR that is a wonderful "little change-big effect" illustration of @Kocal's work ❤️ )

@Kocal Kocal force-pushed the live-component/perf-isLiveComponent branch from cc7f4fa to a7920d4 Compare June 8, 2025 13:06
@Kocal Kocal merged commit aa383ed into symfony:2.x Jun 8, 2025
23 checks passed
symfony-splitter pushed a commit to symfony/ux-live-component that referenced this pull request Jun 8, 2025
…tLiveComponent()` (Kocal)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[LiveComponent] Optimize `LiveComponentStack::getCurrentLiveComponent()`

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Docs?         | no <!-- required for new features -->
| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

Using Reflection during runtime is costly. Here I suggest using static caching to prevent unnecessary usages of Reflection.

I've re-used reproducer from #2812, but with 50 rows (otherwise I can't create a Blackfire Profile), and it reduced the rendering time by 200ms, https://blackfire.io/profiles/compare/7aa62248-9332-40b3-b6fa-58756cb17232/graph
<img width="1341" alt="image" src="https://github.com/user-attachments/assets/d03148c5-521e-4501-937c-54d9e90ece73" />

**EDIT:** see symfony/ux#2821 (comment)

Commits
-------

a7920d429a9 [LiveComponent] Optimize `LiveComponentStack::getCurrentLiveComponent()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix LiveComponent Performance Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants