CoreExtension::getAttribute: small improvement regarding getter/isser/hasser

[gharlan]

For a getter method like getFirstName it is common to call it in twig via person.firstName. But at the moment twig is adding these variants to the class method cache: getFirstName, getfirstname, FirstName and firstname. So when resolving the name, it uses the first elseif here with additional strtolower call, because firstName is missing:

https://github.com/twigphp/Twig/blob/403bd9d73c2a010e5b26689f2f2eb9d7ddf391af/src/Extension/CoreExtension.php#L1863-L1867

This PR replaces FirstName with firstName in the method cache. So person.firstName is resolved via first if branch (but person.FirstName would use the elseif with strtolower now).

[fabpot]

I suppose we're looking for a performance improvement here. Have you tried to measure it because I'm wondering if we could do something like this instead to simplify the code and remove the special cases?

--- 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.

[gharlan]

I suppose we're looking for a performance improvement here.

Yes. Since the optimization is already there, I tried to make it more effective for the common use cases.

Have you tried to measure it because I'm wondering if we could do something like this instead to simplify the code and remove the special cases?

Here is a performance comparison for the attribute reading part (not for the cache building part): https://3v4l.org/Obpbk#v8.4.11

• old is the behavior before this PR
• new is the behavior with the current PR code
• simplified is your proposal
• simplified2 uses the same reduced cache as in your proposal, but still tries array access without strtolower first (which succeeds for all attribute names without upper case letters (street, city…, but not lastName etc.)

Not sure if the differences really matter in real project code. What do you think?