Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:124719
Feedback-ID: ifab94697:Fastmail
Precedence: bulk
MIME-Version: 1.0
Date: Fri, 02 Aug 2024 19:08:36 +0200
To: internals@lists.php.net
Message-ID: <c385b525-37f7-446b-aa96-72272ef9e407@app.fastmail.com>
In-Reply-To: 
 <CAPyj-LCoBY2bPHLmsyONYDbY9aRSraLz5QNskeTiWfpMD0pTSg@mail.gmail.com>
References: 
 <CAPyj-LCoBY2bPHLmsyONYDbY9aRSraLz5QNskeTiWfpMD0pTSg@mail.gmail.com>
Subject: Re: [PHP-DEV] [Concept] Flip relative function lookup order (global, then
 local)
Content-Type: multipart/alternative;
 boundary=db986c3ee94548d495ad1297aca362ca
From: rob@bottled.codes ("Rob Landers")

--db986c3ee94548d495ad1297aca362ca
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable



On Fri, Aug 2, 2024, at 18:51, Ilija Tovilo wrote:
> Hi everyone
>=20
> 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.
>=20
> Not knowing exactly which function is called at compile time has a
> couple of downsides to this:
>=20
> * 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.
>=20
> 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].
>=20
> M=C3=A1t=C3=A9 has kindly benchmarked the patch, measuring an improvem=
ent of
> ~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.

So, what you=E2=80=99re saying is that symfony and laravel can get a per=
formance increase by simply adding a \ in the right places? Why don=E2=80=
=99t they do that instead of changing the language?

>=20
> There are a few noteworthy downsides:
>=20
> * 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.

For functions/classes in the same exact namespace, you don=E2=80=99t nee=
d a use statement. But after this change, you do in certain cases?

namespace Foo;

function array_sum($bar) {}

function baz($bar) {
  return array_sum($bar);
}

So, how do you use that function in the same 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.

We can only see open source code when doing impact analysis. This means =
picking even a slightly =E2=80=9Cpopular=E2=80=9D name could go very poo=
rly.

> * 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.

See above. I=E2=80=99ve seen this =E2=80=9Ctrick=E2=80=9D used on many c=
losed source projects. I=E2=80=99ve also seen it used when PHP has a bug=
 and the workaround is to implement it in php like this.=20

>=20
> 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.
>=20
> 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.
>=20
> 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).
>=20
> 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.
>=20
> Ilija
>=20
> [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/Php=
Unit/ClockMock.php
> [6] https://gist.github.com/iluuu1994/4b83481baac563f8f0d3204c697c5551
>=20

=E2=80=94 Rob
--db986c3ee94548d495ad1297aca362ca
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE html><html><head><title></title><style type=3D"text/css">p.Mso=
Normal,p.MsoNoSpacing{margin:0}</style></head><body><div><br></div><div>=
<br></div><div>On Fri, Aug 2, 2024, at 18:51, Ilija Tovilo wrote:<br></d=
iv><blockquote type=3D"cite" id=3D"qt" style=3D""><div>Hi everyone<br></=
div><div><br></div><div>As you probably know, a common performance optim=
ization in PHP is to<br></div><div>prefix global function calls in names=
paced code with a `\`. In<br></div><div>namespaced code, relative functi=
on calls (meaning, not prefixed with<br></div><div>`\`, not imported and=
 not containing multiple namespace components)<br></div><div>will be loo=
ked up in the current namespace before falling back to the<br></div><div=
>global namespace. Prefixing the function name with `\` disambiguates<br=
></div><div>the called function by always picking the global function.<b=
r></div><div><br></div><div>Not knowing exactly which function is called=
 at compile time has a<br></div><div>couple of downsides to this:<br></d=
iv><div><br></div><div>* It leads to the aforementioned double-lookup.<b=
r></div><div>* It prevents compile-time-evaluation of pure internal func=
tions.<br></div><div>* It prevents compiling to specialized&nbsp; opcode=
s for specialized<br></div><div>internal functions (e.g. strlen()).<br><=
/div><div>* It requires branching for frameless functions [1].<br></div>=
<div>* It prevents an optimization that looks up internal functions by<b=
r></div><div>offset rather than by name [2].<br></div><div>* It prevents=
 compiling to more specialized argument sending opcodes<br></div><div>be=
cause of unknown by-value/by-reference passing.<br></div><div><br></div>=
<div>All of these are enabled by disambiguating the call. Unfortunately,=
<br></div><div>prefixing all calls with `\`, or adding a `use function` =
at the top of<br></div><div>every file is annoying and noisy. We recentl=
y got a feature request to<br></div><div>change how functions are looked=
 up [3]. The approach that appears to<br></div><div>cause the smallest b=
ackwards incompatibility is to flip the order in<br></div><div>which fun=
ctions are looked up: Check in global scope first, and only<br></div><di=
v>then in local scope. With this approach, if we can find a global<br></=
div><div>function at compile-time, we know this is the function that wil=
l be<br></div><div>picked at run-time, hence automatically enabling the =
optimizations<br></div><div>above. I created a PoC implementing this app=
roach [4].<br></div><div><br></div><div>M=C3=A1t=C3=A9 has kindly benchm=
arked the patch, measuring an improvement of<br></div><div>~3.9% for Lar=
avel, and ~2.1% for Symfony<br></div><div>(<a href=3D"https://gist.githu=
b.com/kocsismate/75be09bf6011630ebd40a478682d6c17">https://gist.github.c=
om/kocsismate/75be09bf6011630ebd40a478682d6c17</a>).<br></div><div>This =
seems quite significant, given that no changes were required in<br></div=
><div>either of these two codebases.<br></div></blockquote><div><br></di=
v><div>So, what you=E2=80=99re saying is that symfony and laravel can ge=
t a performance increase by simply adding a \ in the right places? Why d=
on=E2=80=99t they do that instead of changing the language?<br></div><di=
v><br></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div><br></di=
v><div>There are a few noteworthy downsides:<br></div><div><br></div><di=
v>* Unqualified calls to functions in the same namespace would be<br></d=
iv><div>slightly slower, because they now involve checking global scope =
first.<br></div><div>I believe that unqualified, global calls are much m=
ore common, so this<br></div><div>change should still result in a net po=
sitive. It's also possible to<br></div><div>avoid this cost by adding a =
`use function` to the top of the file.<br></div></blockquote><div><br></=
div><div>For functions/classes in the same exact namespace, you don=E2=80=
=99t need a use statement. But after this change, you do in certain case=
s?<br></div><div><br></div><div>namespace Foo;<br></div><div><br></div><=
div>function array_sum($bar) {}<br></div><div><br></div><div>function ba=
z($bar) {<br></div><div>&nbsp; return array_sum($bar);<br></div><div>}<b=
r></div><div><br></div><div>So, how do you use that function in the same=
 file?</div><div><br></div><blockquote type=3D"cite" id=3D"qt" style=3D"=
"><div>* Introducing new functions in the global namespace could cause a=
 BC<br></div><div>break for unqualified calls, if the function happens t=
o have the same<br></div><div>name. This is unfortunate, but likely rare=
. Since new functions are<br></div><div>only introduced in minor/major v=
ersions, this should be manageable,<br></div><div>but must be considered=
 for every PHP upgrade.<br></div></blockquote><div><br></div><div>We can=
 only see open source code when doing impact analysis. This means pickin=
g even a slightly =E2=80=9Cpopular=E2=80=9D name could go very poorly.</=
div><div><br></div><blockquote type=3D"cite" id=3D"qt" style=3D""><div>*=
 Some mocking libraries (e.g. Symfony's ClockMock [5]) intentionally<br>=
</div><div>declare functions called from some file in the files namespac=
e to<br></div><div>intercept these calls. This use-case would break. Tha=
t said, it is<br></div><div>somewhat of a fragile approach to begin with=
, given that it wouldn't<br></div><div>work for fully qualified calls, o=
r unnamespaced code.<br></div></blockquote><div><br></div><div>See above=
. I=E2=80=99ve seen this =E2=80=9Ctrick=E2=80=9D used on many closed sou=
rce projects. I=E2=80=99ve also seen it used when PHP has a bug and the =
workaround is to implement it in php like this.&nbsp;</div><div><br></di=
v><blockquote type=3D"cite" id=3D"qt" style=3D""><div><br></div><div>I p=
erformed a small impact analysis [6]. There are 484 namespaced<br></div>=
<div>functions shadowing global, internal functions in the top 1000<br><=
/div><div>composer packages. However, the vast majority (464) of these f=
unctions<br></div><div>come from thecodingmachine/safe, whose entire pur=
pose is offering<br></div><div>safer wrappers around internal functions.=
 Excluding this library,<br></div><div>there are only 20 shadowing funct=
ions, which is surprisingly little.<br></div><div>Furthermore, the patch=
 would have no impact on users of<br></div><div>thecodingmachine/safe, o=
nly on the library code itself.<br></div><div><br></div><div>As for prov=
iding a migration path: One approach might be to introduce<br></div><div=
>an INI setting that performs the function lookup in both local and<br><=
/div><div>global scope at run-time, and informs the user about the behav=
ioral<br></div><div>change in the future. To mitigate it, an explicit `u=
se function` would<br></div><div>need to be added to the top of the file=
, or the call would need to be<br></div><div>prefixed with `namespace\`.=
 The impact analysis [6] also provides a<br></div><div>script that looks=
 for shadowing functions in your project. It does not<br></div><div>iden=
tify uses of these functions (yet), just their declarations.<br></div><d=
iv><br></div><div>Lastly, I've already raised this idea in the PHP Found=
ations internal<br></div><div>chat but did not receive much positive fee=
dback, mostly due to fear of<br></div><div>the potential BC impact. I'm =
not particularly convinced this is an<br></div><div>issue, given the imp=
act analysis. Given the surprisingly large<br></div><div>performance ben=
efits, I was inclined to raise it here anyway. It also<br></div><div>spa=
rked some related ideas, like providing modules that lock<br></div><div>=
namespaces and optimize multiple files as a singular unit. That said,<br=
></div><div>such approaches would likely be significantly more complex t=
han the<br></div><div>approach proposed here (~30 lines of C code).<br><=
/div><div><br></div><div>Anyway, please let me know about possible conce=
rns, broken use-cases,<br></div><div>or any alternative approaches that =
may come to mind. I'm looking<br></div><div>forward to your feedback.<br=
></div><div><br></div><div>Ilija<br></div><div><br></div><div>[1]&nbsp;<=
a href=3D"https://github.com/php/php-src/pull/12461">https://github.com/=
php/php-src/pull/12461</a><br></div><div>[2]&nbsp;<a href=3D"https://git=
hub.com/php/php-src/pull/13634">https://github.com/php/php-src/pull/1363=
4</a><br></div><div>[3]&nbsp;<a href=3D"https://github.com/php/php-src/i=
ssues/13632">https://github.com/php/php-src/issues/13632</a><br></div><d=
iv>[4]&nbsp;<a href=3D"https://github.com/php/php-src/pull/14529">https:=
//github.com/php/php-src/pull/14529</a><br></div><div>[5]&nbsp;<a href=3D=
"https://github.com/symfony/symfony/blob/7.1/src/Symfony/Bridge/PhpUnit/=
ClockMock.php">https://github.com/symfony/symfony/blob/7.1/src/Symfony/B=
ridge/PhpUnit/ClockMock.php</a><br></div><div>[6]&nbsp;<a href=3D"https:=
//gist.github.com/iluuu1994/4b83481baac563f8f0d3204c697c5551">https://gi=
st.github.com/iluuu1994/4b83481baac563f8f0d3204c697c5551</a><br></div><d=
iv><br></div></blockquote><div><br></div><div id=3D"sig121229152">=E2=80=
=94 Rob<br></div></body></html>
--db986c3ee94548d495ad1297aca362ca--