Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
At DrupalCon Nashville, a presentation on Symfony 4 PHP performance improvements included a discussion of the OPcache compilation process and the relatively easy/straightforward changes Symfony made in commonly-used classes to benefit from this. (These upstream optimizations have already landed in Drupal as part of Symfony 3.4.)
In a nutshell, OPcache will better optimize calls to native PHP functions like is_array() within namespaced code only if root-namespace prefixed, e.g., \is_array(), because at runtime the non-namespaced, non-native variant may exist. This is not commonly done by developers at the moment, though the Drupal coding standards do not appear to specifically preclude it, either:
Functions should be called with no spaces between the function name, the opening parenthesis, and the first parameter; spaces between commas and each parameter, and no space between the last parameter, the closing parenthesis, and the semicolon. Here's an example:
I have not yet profiled the improvements that Drupal may see from such refactoring, but given Symfony's (and others, based on the GitHub inbound issue mentions) adoption of this pattern, I would imagine it is more than negligible.
Proposed resolution
Apply a targeted application of this pattern and (potentially?) encourage it by way of the coding standards or other documentation.
Remaining tasks
Get buy-in from core committers and profile performance improvements.
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | test-before.png | 86.36 KB | ever-c0de |
| #3 | root-prefixed.patch | 545.32 KB | bradjones1 |
| #2 | root-prefixed.patch | 545.32 KB | bradjones1 |
Issue fork drupal-2960522
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
bradjones1Attached is a POC patch generated with:
vendor/bin/php-cs-fixer fix lib --rules='{"native_function_invocation":{"opcache-only": true}}' --allow-risky=yes(requires the option added in this PR)
Comment #3
bradjones1Ack, I screwed up the prefixes and suffixes on the git diff.
Comment #4
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedWe should do this provided we can measure improvements and implement an automated standards check.
Comment #5
bradjones1@cilefen Are you at the sprints today in Nashville? Let's talk about the best plan of attack there? I am not an expert at profiling but I do have a Blackfire subscription and perhaps there's a way to run the core test suite with and without this type of patch and that would be sufficient? Or I'm not sure what other kind of more "real-world" profile might make more sense?
Comment #6
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedI am not there. It's probably worth something on its own if it only improves test performance. But real use-cases are what we mainly want to see improving. I am on Slack.
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIt would probably make sense to do these changes only in the files that are in the hot path of a request, before any caching kicks in, not everywhere across the board like the current patch does.
Comment #8
heddnI'm queueing a test, but I want to say that namespacing these things will not work on php 5.5. I'm not sure though, so testing it.
Comment #9
heddnLooks like I'm wrong. See https://3v4l.org/gdmrW. Seems like it was added in php 5.4.
Comment #10
dawehnerI think @amateescu's suggestion is great! I would easily imagine that the the result is kind of disappointing though.
Comment #11
bradjones1It's certainly possible that either a) our specific implementation does not benefit much from this or b) the changes from Symfony along the same vein accomplished whatever optimization we might have otherwise obtained. Some initial testing with ab would indicate there's not much change, however I am going to have to devise a better profiling plan than just a bare-bones install with opcache installed, since in my local environment the requests are around 5 ms, which doesn't give much room for improvement, anyway. Ideas would be welcome.
Comment #12
dawehner@bradjones1 I would suggest you to use maybe tools like xhprof or blackfire. They tend to show a better picture, especially in terms of understanding why something got faster.
Comment #20
ever-c0deHello, guys.
It's been a long time since the last comment so I decided to repeat the test.
I have tested difference between the default core and the prefixes appended to built-in functions.
I did the following setup:
(idea was to have different components that should be rendered)
I used admin user and disabled all render cache.
Test before prefixes appended:
Link to the test.
Test after prefixes appended:
Link to the test.
We can compare those two tests:
Link.
The difference is in -4.0-5.1%.
In total I have made 3 tests:
First test.
Second test.
Third test.
To append prefixes to built-in functions I have used `php-cs-fixer` with this command:
vendor/bin/php-cs-fixer fix core --rules='{"native_function_invocation":{"scope": namespaced}}' --allow-risky=yes(it has changed since comment #2)Comment #22
MatroskeenWe were working on this during our local code sprint event, so adding a tag :)
Comment #23
MatroskeenAt a glance, numbers are promising, but it would be great to run more tests.
I think they might be similar to those performed by @Berdir here: #2407187-38: Optimize LibraryDependencyResolver::getMinimalRepresentativeSubset() and win >=4%.
Also, one test is failing, so I'm moving to NW at this moment.