-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
CoreExtension::getAttribute: small improvement regarding getter/isser/hasser #4662
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
base: 3.x
Are you sure you want to change the base?
Conversation
b173e57 to
f8d742e
Compare
f8d742e to
45cd6ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes the method name caching in Twig's CoreExtension by improving how getter/is/has method names are cached to reduce unnecessary string operations during attribute resolution. The main goal is to cache method names in their camelCase form (e.g., firstName instead of FirstName) so that common property access patterns like person.firstName can be resolved more efficiently.
Key changes:
- Refactored prefix handling for getter/is/has methods to use a common
$prefixLengthvariable - Modified the cache key generation to store camelCase names (e.g.,
firstName) instead of PascalCase names (e.g.,FirstName) - Updated comments to reflect the improved caching strategy
|
I suppose we're looking for a performance improvement here. --- a/src/Extension/CoreExtension.php
+++ b/src/Extension/CoreExtension.php
@@ -1826,17 +1826,13 @@ final class CoreExtension extends AbstractExtension
$lcMethods = array_map('strtolower', $methods);
$classCache = [];
foreach ($methods as $i => $method) {
- $classCache[$method] = $method;
$classCache[$lcName = $lcMethods[$i]] = $method;
if ('g' === $lcName[0] && str_starts_with($lcName, 'get')) {
- $name = substr($method, 3);
$lcName = substr($lcName, 3);
} elseif ('i' === $lcName[0] && str_starts_with($lcName, 'is')) {
- $name = substr($method, 2);
$lcName = substr($lcName, 2);
} elseif ('h' === $lcName[0] && str_starts_with($lcName, 'has')) {
- $name = substr($method, 3);
$lcName = substr($lcName, 3);
if (\in_array('is'.$lcName, $lcMethods, true)) {
continue;
@@ -1846,23 +1842,15 @@ final class CoreExtension extends AbstractExtension
}
// skip get() and is() methods (in which case, $name is empty)
- if ($name) {
- if (!isset($classCache[$name])) {
- $classCache[$name] = $method;
- }
-
- if (!isset($classCache[$lcName])) {
- $classCache[$lcName] = $method;
- }
+ if ($lcName && !isset($classCache[$lcName])) {
+ $classCache[$lcName] = $method;
}
}
$cache[$class] = $classCache;
}
$call = false;
- if (isset($cache[$class][$item])) {
- $method = $cache[$class][$item];
- } elseif (isset($cache[$class][$lcItem = strtolower($item)])) {
+ if (isset($cache[$class][$lcItem = strtolower($item)])) {
$method = $cache[$class][$lcItem];
} elseif (isset($cache[$class]['__call'])) {
$method = $item;Memory should be a bit less and I expect the performance to be (almost) the same. |
Yes. Since the optimization is already there, I tried to make it more effective for the common use cases.
Here is a performance comparison for the attribute reading part (not for the cache building part): https://3v4l.org/Obpbk#v8.4.11
Not sure if the differences really matter in real project code. What do you think? |
For a getter method like
getFirstNameit is common to call it in twig viaperson.firstName.But at the moment twig is adding these variants to the class method cache:
getFirstName,getfirstname,FirstNameandfirstname.So when resolving the name, it uses the first
elseifhere with additionalstrtolowercall, becausefirstNameis missing:Twig/src/Extension/CoreExtension.php
Lines 1863 to 1867 in 403bd9d
This PR replaces
FirstNamewithfirstNamein the method cache.So
person.firstNameis resolved via firstifbranch (butperson.FirstNamewould use theelseifwithstrtolowernow).