Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:124718
Precedence: bulk
MIME-Version: 1.0
Date: Fri, 2 Aug 2024 18:51:33 +0200
Message-ID: <CAPyj-LCoBY2bPHLmsyONYDbY9aRSraLz5QNskeTiWfpMD0pTSg@mail.gmail.com>
Subject: [PHP-DEV] [Concept] Flip relative function lookup order (global, then local)
To: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: tovilo.ilija@gmail.com (Ilija Tovilo)

Hi everyone

As you probably know, a common performance optimization in PHP is to
prefix global function calls in namespaced code with a `\`. In
namespaced code, relative function calls (meaning, not prefixed with
`\`, not imported and not containing multiple namespace components)
will be looked up in the current namespace before falling back to the
global namespace. Prefixing the function name with `\` disambiguates
the called function by always picking the global function.

Not knowing exactly which function is called at compile time has a
couple of downsides to this:

* It leads to the aforementioned double-lookup.
* It prevents compile-time-evaluation of pure internal functions.
* It prevents compiling to specialized  opcodes for specialized
internal functions (e.g. strlen()).
* It requires branching for frameless functions [1].
* It prevents an optimization that looks up internal functions by
offset rather than by name [2].
* It prevents compiling to more specialized argument sending opcodes
because of unknown by-value/by-reference passing.

All of these are enabled by disambiguating the call. Unfortunately,
prefixing all calls with `\`, or adding a `use function` at the top of
every file is annoying and noisy. We recently got a feature request to
change how functions are looked up [3]. The approach that appears to
cause the smallest backwards incompatibility is to flip the order in
which functions are looked up: Check in global scope first, and only
then in local scope. With this approach, if we can find a global
function at compile-time, we know this is the function that will be
picked at run-time, hence automatically enabling the optimizations
above. I created a PoC implementing this approach [4].

M=C3=A1t=C3=A9 has kindly benchmarked the patch, measuring an improvement o=
f
~3.9% for Laravel, and ~2.1% for Symfony
(https://gist.github.com/kocsismate/75be09bf6011630ebd40a478682d6c17).
This seems quite significant, given that no changes were required in
either of these two codebases.

There are a few noteworthy downsides:

* Unqualified calls to functions in the same namespace would be
slightly slower, because they now involve checking global scope first.
I believe that unqualified, global calls are much more common, so this
change should still result in a net positive. It's also possible to
avoid this cost by adding a `use function` to the top of the file.
* Introducing new functions in the global namespace could cause a BC
break for unqualified calls, if the function happens to have the same
name. This is unfortunate, but likely rare. Since new functions are
only introduced in minor/major versions, this should be manageable,
but must be considered for every PHP upgrade.
* Some mocking libraries (e.g. Symfony's ClockMock [5]) intentionally
declare functions called from some file in the files namespace to
intercept these calls. This use-case would break. That said, it is
somewhat of a fragile approach to begin with, given that it wouldn't
work for fully qualified calls, or unnamespaced code.

I performed a small impact analysis [6]. There are 484 namespaced
functions shadowing global, internal functions in the top 1000
composer packages. However, the vast majority (464) of these functions
come from thecodingmachine/safe, whose entire purpose is offering
safer wrappers around internal functions. Excluding this library,
there are only 20 shadowing functions, which is surprisingly little.
Furthermore, the patch would have no impact on users of
thecodingmachine/safe, only on the library code itself.

As for providing a migration path: One approach might be to introduce
an INI setting that performs the function lookup in both local and
global scope at run-time, and informs the user about the behavioral
change in the future. To mitigate it, an explicit `use function` would
need to be added to the top of the file, or the call would need to be
prefixed with `namespace\`. The impact analysis [6] also provides a
script that looks for shadowing functions in your project. It does not
identify uses of these functions (yet), just their declarations.

Lastly, I've already raised this idea in the PHP Foundations internal
chat but did not receive much positive feedback, mostly due to fear of
the potential BC impact. I'm not particularly convinced this is an
issue, given the impact analysis. Given the surprisingly large
performance benefits, I was inclined to raise it here anyway. It also
sparked some related ideas, like providing modules that lock
namespaces and optimize multiple files as a singular unit. That said,
such approaches would likely be significantly more complex than the
approach proposed here (~30 lines of C code).

Anyway, please let me know about possible concerns, broken use-cases,
or any alternative approaches that may come to mind. I'm looking
forward to your feedback.

Ilija

[1] https://github.com/php/php-src/pull/12461
[2] https://github.com/php/php-src/pull/13634
[3] https://github.com/php/php-src/issues/13632
[4] https://github.com/php/php-src/pull/14529
[5] https://github.com/symfony/symfony/blob/7.1/src/Symfony/Bridge/PhpUnit/=
ClockMock.php
[6] https://gist.github.com/iluuu1994/4b83481baac563f8f0d3204c697c5551