Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126163
Precedence: bulk
MIME-Version: 1.0
References: <27531d9d-9bfe-4acc-b9ab-80b1017e3038@app.fastmail.com>
 <CAEKnhAFU32EXh483QzR0LLUt+Jou4ejKjOXqBQ_G43wR7LW5cA@mail.gmail.com> <e8736456-218d-4a55-a595-e1346e1c6d20@app.fastmail.com>
In-Reply-To: <e8736456-218d-4a55-a595-e1346e1c6d20@app.fastmail.com>
Date: Sun, 22 Dec 2024 08:48:02 +0100
Message-ID: <CAEKnhAEAXCmnqfCEyqJbJWr-=XRDY31m2eJDA3WmX9HcA8a0kg@mail.gmail.com>
Subject: Re: [PHP-DEV] Discussion: Remove file statcache?
To: Larry Garfield <larry@garfieldtech.com>
Cc: php internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000002254480629d71a6d"
From: bukka@php.net (Jakub Zelenka)

--0000000000002254480629d71a6d
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Dec 22, 2024 at 5:12=E2=80=AFAM Larry Garfield <larry@garfieldtech.=
com>
wrote:

> On Sat, Dec 21, 2024, at 10:43 AM, Jakub Zelenka wrote:
> > On Fri, Dec 20, 2024 at 8:29=E2=80=AFPM Larry Garfield <larry@garfieldt=
ech.com>
> wrote:
> >> Background: PHP has a not-often-considered feature, the stat-cache.
> That is, the runtime caches the OS stat() call for files, so that
> subsequent reads on the same file can be faster.  However, it's even less
> realized that it's a single-file cache.  It literally only applies when y=
ou
> try to do two file-infomation operations on the same file in rapid
> succession, without any other file reads in between.
>
> > I would prefer to disable it by default but keep some option (INI) to
> > re-enable it. I think that for most users the perf impact will be
> > negligible. However, it is quite likely that there are some user
> > workflows and platforms where benefiting from the stat cache can be
> > still significant in terms of performance. So those users should have
> > the option to re-enable it if they see some significant regression
> > rather then force them to update their code to make it faster or
> > implement their own cache which would just make their migration to the
> > next version much harder / potentially impossible. There is not such a
> > huge maintenance that we would really need to get rid of it completely.
> > I would really prefer having such option and tell to users to re-enable
> > it rather than not be able to deal with potentially reported future
> > perf regressions.
> >
> > I think the main issue with the cache is that is just not convenient
> > for use cases where it doesn't get flushed during some different access
> > methods that don't trigger flush. We could probably improve the stream
> > situation a bit but it still leaves external (e.g. shell) access
> > problem in place which we just cannot fix. On the other hand it is
> > possible to use it in a way that users can profit from it but they
> > really need to know how it works. That's way it should be an optional
> > feature IMO. We should also improve documentation in that regards.
> >
> > In terms of voting, if there was no option to re-enable it, I would
> > probably vote against this proposal as I'm a bit worried about those
> > possible regression reports.
>
> I really don't like the idea of another ini toggle.  That actually create=
s
> more work, as people writing code that works with the file system now hav=
e
> one more invisible context they have to think about.  Which means they
> probably won't, until it bites them.  (They'll either never bother cleari=
ng
> the cache, so their code may malfunction on the rare system where it's
> enabled, or always clear it, which 99.9% of the time will actually be
> slower as we have to invoke the function for it to do nothing.  Both are
> bad.)
>
>
Well it's much less likely to bite anyone than if it's always on. I think
if we document it well and there is a good switch note, it should be clear
enough for users and only users that understand what it does should enable
it.

I can see that if anyone enables it just on prod, then they will have hard
time to recreate the issues on local setup but that's already the case with
some other option. You just need to get the right settings from prod to be
able to recreate things on local setup.

I don't really have a better idea how to minimize impact on the users if
they see significant regression from this change. Changing the functions
signature is just not viable IMO.


> I suppose a possible alternative would be to modify all file system
> mutation functions (file_put_contents(), touch(), etc.) to flush the cach=
e,
> which for whatever reason doesn't happen now.  That would be above my ski=
ll
> level, though, so someone else would need to do it.  Also, I don't know i=
f
> there's a good reason those functions don't clear the cache currently or =
if
> it was just an oversight.
>
>
As I said we could probably handle some stream cases more aggressively but
it won't resolve the problem completely. We still have things like
system("touch /file/path") which we cannot flush the stat cache for. And
it's not just shell access - there might be some 3rd party extensions that
operate on files or there might be other programs accessing files at the
same time. So there are many places which we just cannot control.

Regards

Jakub

--0000000000002254480629d71a6d
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div>On Sun, Dec 22, 2024 at 5:12=E2=80=AFAM Larry Garfiel=
d &lt;<a href=3D"mailto:larry@garfieldtech.com">larry@garfieldtech.com</a>&=
gt; wrote:</div><div class=3D"gmail_quote gmail_quote_container"><blockquot=
e class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px s=
olid rgb(204,204,204);padding-left:1ex">On Sat, Dec 21, 2024, at 10:43 AM, =
Jakub Zelenka wrote:<br>
&gt; On Fri, Dec 20, 2024 at 8:29=E2=80=AFPM Larry Garfield &lt;<a href=3D"=
mailto:larry@garfieldtech.com" target=3D"_blank">larry@garfieldtech.com</a>=
&gt; wrote:<br>
&gt;&gt; Background: PHP has a not-often-considered feature, the stat-cache=
.=C2=A0 That is, the runtime caches the OS stat() call for files, so that s=
ubsequent reads on the same file can be faster.=C2=A0 However, it&#39;s eve=
n less realized that it&#39;s a single-file cache.=C2=A0 It literally only =
applies when you try to do two file-infomation operations on the same file =
in rapid succession, without any other file reads in between.<br>
<br>
&gt; I would prefer to disable it by default but keep some option (INI) to =
<br>
&gt; re-enable it. I think that for most users the perf impact will be <br>
&gt; negligible. However, it is quite likely that there are some user <br>
&gt; workflows and platforms where benefiting from the stat cache can be <b=
r>
&gt; still significant in terms of performance. So those users should have =
<br>
&gt; the option to re-enable it if they see some significant regression <br=
>
&gt; rather then force them to update their code to make it faster or <br>
&gt; implement their own cache which would just make their migration to the=
 <br>
&gt; next version much harder / potentially impossible. There is not such a=
 <br>
&gt; huge maintenance that we would really need to get rid of it completely=
. <br>
&gt; I would really prefer having such option and tell to users to re-enabl=
e <br>
&gt; it rather than not be able to deal with potentially reported future <b=
r>
&gt; perf regressions. <br>
&gt;<br>
&gt; I think the main issue with the cache is that is just not convenient <=
br>
&gt; for use cases where it doesn&#39;t get flushed during some different a=
ccess <br>
&gt; methods that don&#39;t trigger flush. We could probably improve the st=
ream <br>
&gt; situation a bit but it still leaves external (e.g. shell) access <br>
&gt; problem in place which we just cannot fix. On the other hand it is <br=
>
&gt; possible to use it in a way that users can profit from it but they <br=
>
&gt; really need to know how it works. That&#39;s way it should be an optio=
nal <br>
&gt; feature IMO. We should also improve documentation in that regards.<br>
&gt;<br>
&gt; In terms of voting, if there was no option to re-enable it, I would <b=
r>
&gt; probably vote against this proposal as I&#39;m a bit worried about tho=
se <br>
&gt; possible regression reports.<br>
<br>
I really don&#39;t like the idea of another ini toggle.=C2=A0 That actually=
 creates more work, as people writing code that works with the file system =
now have one more invisible context they have to think about.=C2=A0 Which m=
eans they probably won&#39;t, until it bites them.=C2=A0 (They&#39;ll eithe=
r never bother clearing the cache, so their code may malfunction on the rar=
e system where it&#39;s enabled, or always clear it, which 99.9% of the tim=
e will actually be slower as we have to invoke the function for it to do no=
thing.=C2=A0 Both are bad.)<br>
<br></blockquote><div><br></div><div>Well it&#39;s much less likely to bite=
 anyone than if it&#39;s always on. I think if we document it well and ther=
e is a good switch note, it should be clear enough for users and only users=
 that understand what it does should enable it.</div><div><br></div><div>I =
can see that if anyone enables it just on prod, then they will have hard ti=
me to recreate the issues on local setup but that&#39;s already the case wi=
th some other option. You just need to get the right settings from prod to =
be able to recreate things on local setup.=C2=A0</div><div><br></div><div>I=
 don&#39;t really have a better idea how to minimize impact on the users if=
 they see significant regression from this change. Changing the functions s=
ignature is just not viable IMO.</div><div>=C2=A0</div><blockquote class=3D=
"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(2=
04,204,204);padding-left:1ex">
I suppose a possible alternative would be to modify all file system mutatio=
n functions (file_put_contents(), touch(), etc.) to flush the cache, which =
for whatever reason doesn&#39;t happen now.=C2=A0 That would be above my sk=
ill level, though, so someone else would need to do it.=C2=A0 Also, I don&#=
39;t know if there&#39;s a good reason those functions don&#39;t clear the =
cache currently or if it was just an oversight.<br><br></blockquote><div><b=
r></div><div>As I said we could probably handle some stream cases more aggr=
essively but it won&#39;t resolve the problem completely. We still have thi=
ngs like system(&quot;touch /file/path&quot;) which we cannot flush the sta=
t cache for. And it&#39;s not just shell access - there might be some 3rd p=
arty extensions that operate on files or there might be other programs acce=
ssing files at the same time. So there are many places which we just cannot=
 control.</div><div><br></div><div>Regards=C2=A0 =C2=A0</div><div><br></div=
><div>Jakub</div></div></div>

--0000000000002254480629d71a6d--