[TwigComponent] Minor performance improvement by caching PropertyAccessor::isWritable() calls#3341
Conversation
…essor::isWritable()` calls
carsonbot
changed the title
PropertyAccessor::isWritable() callsPropertyAccessor::isWritable() calls
There was a problem hiding this comment.
Pull request overview
This PR introduces a small cache inside ComponentFactory to avoid repeated PropertyAccessor::isWritable() checks when mounting many instances of the same Twig component, aiming to reduce overhead during repeated renders.
Changes:
- Add an in-memory cache for
PropertyAccessorInterface::isWritable()results keyed by component name and property. - Clear the new cache in
reset()alongside existing cached reflection data.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach ($data as $property => $value) { | ||
| if ($this->propertyAccessor->isWritable($component, $property)) { | ||
| if ($this->writableProperties[$componentMetadata->getName()][$property] ??= $this->propertyAccessor->isWritable($component, $property)) { | ||
| $this->propertyAccessor->setValue($component, $property, $value); |
There was a problem hiding this comment.
$data at this point can include arbitrary attribute keys (e.g. class, data-*, spread attributes), not just actual component properties. Caching writability for every encountered key can cause the cache to grow with the number of distinct attribute names used during the request/worker lifetime. Consider limiting what gets cached (e.g. only cache keys that are valid property names / property paths you explicitly support, or only cache positive isWritable() results), and/or key the cache by the component class (since writability is effectively a class+property concern).
There was a problem hiding this comment.
That's true, but I think it's fine
carsonbot
changed the title
PropertyAccessor::isWritable() callsPropertyAccessor::isWritable() calls
kbond
left a comment
kbond
left a comment
There was a problem hiding this comment.
Looks good. I would have thought the property accessor had its own cache.
carsonbot
added
Status: Reviewed
|
It does, but for computing the property access path IIRC. |
3 participants
Still on Kocal/Gotta-Catch-Em-All#11, my
Pokemoncomponent has three properties and is rendered 386 times in the page.We can remove 1155 calls to
PropertyAccessor::isWritable()by caching the result of the first call for each property of the component instance. This number can increase of each attributes passed to the component.AFAIK
PropertyAccessor::isWritable($componentInstance, $propertyName)can not return two different values for the same$componentInstance(an object) and$propertyName(a property of the object, or an HTML attr key) arguments.