Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:126247
Feedback-ID: ifab94697:Fastmail
Precedence: bulk
MIME-Version: 1.0
Date: Thu, 30 Jan 2025 19:11:00 +0100
To: "Arkadiy Kulev" <eth@ethaniel.com>, "Jakub Zelenka" <bukka@php.net>
Cc: internals@lists.php.net
Message-ID: <3624b46b-16f2-4eb3-b1ac-d0439cd79153@app.fastmail.com>
In-Reply-To: 
 <CAGVaEcnOWGYPPUTYn1OZkxCvLmXSnzrQm73YB=ucRMuOA6DCpA@mail.gmail.com>
References: 
 <CAGVaEcmDxxbFzNxXnF=jmdUDnQTk55zgorbsqQqyP8w2Z=-iaw@mail.gmail.com>
 <CAEKnhAFeiJESkqJ0o9PuU59VFK1hdfk2xeEDJn_BbsaF0sBy7Q@mail.gmail.com>
 <CAGVaEcnOWGYPPUTYn1OZkxCvLmXSnzrQm73YB=ucRMuOA6DCpA@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] Introducing pm.max_memory for PHP-FPM
Content-Type: multipart/alternative;
 boundary=5f8ee6bb1e224c93904d3ea73b7d8a8a
From: rob@bottled.codes ("Rob Landers")

--5f8ee6bb1e224c93904d3ea73b7d8a8a
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 30, 2025, at 17:41, Arkadiy Kulev wrote:
> Hello!
>> This wouldn't really work because FPM does not control the script dur=
ing execution and would have to check it out after each allocation which=
 is not really viable.
>=20
> Thanks for the feedback! I agree that monitoring memory usage after ea=
ch allocation would be infeasible. However, my suggestion was actually t=
o check memory usage *only once per request*, specifically at request sh=
utdown, when FPM regains control and before assigning another request to=
 that worker.=20
>=20
> We already have hooks for request startup and request shutdown in FPM.=
 Could we simply insert a memory check at the request shutdown stage=E2=80=
=94where the worker is returning control to the FPM master process=E2=80=
=94before picking up a new request?
>=20
> Just to be clear, "memory_limit" helps kill runaway scripts mid-reques=
t. By contrast, the newly proposed pm.max_memory is meant to catch proce=
sses with a slow leak across multiple requests. We only need to check at=
 the end of each request, which is presumably when the worker returns co=
ntrol to FPM.

To be honest, I haven't seen a 'slow' memory leak in a long time -- exce=
pt when using fgetcsv which has had a reported memory leak since ~2012 s=
omewhere on the old bug tracker. (I lost the link to it and don't even r=
emember how to get to the tracker or if it still exists.) I haven't chec=
ked if it has been fixed since 8.2, but I've seen a couple of reports of=
 it on r/php a couple of times in the last couple of years.

=E2=80=94 Rob
--5f8ee6bb1e224c93904d3ea73b7d8a8a
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>On Thu, Jan 30,=
 2025, at 17:41, Arkadiy Kulev wrote:<br></div><blockquote type=3D"cite"=
 id=3D"qt" style=3D""><div dir=3D"ltr"><div dir=3D"ltr"><div><div>Hello!=
<br></div></div></div><div class=3D"qt-gmail_quote qt-gmail_quote_contai=
ner"><blockquote class=3D"qt-gmail_quote" style=3D"margin-top:0px;margin=
-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;bor=
der-left-style:solid;border-left-color:rgb(204, 204, 204);padding-left:1=
ex;"><div dir=3D"ltr"><div class=3D"qt-gmail_quote"><div>This wouldn't r=
eally work because FPM does not control the script during execution and =
would have to check it out after each allocation which is not really via=
ble.<br></div></div></div></blockquote><div><div><br></div><div>Thanks f=
or the feedback! I agree that monitoring memory usage after each allocat=
ion would be infeasible. However, my suggestion was actually to check me=
mory usage <b>only once per request</b>, specifically at request shutdow=
n, when FPM regains control and before assigning another request to that=
 worker.&nbsp;<br></div></div><div><br></div><div>We already have hooks =
for request startup and request shutdown in FPM. Could we simply insert =
a memory check at the request shutdown stage=E2=80=94where the worker is=
 returning control to the FPM master process=E2=80=94before picking up a=
 new request?<br></div><div><br></div><div>Just to be clear, "memory_lim=
it" helps kill runaway scripts mid-request. By contrast, the newly propo=
sed pm.max_memory is meant to catch processes with a slow leak across mu=
ltiple requests. We only need to check at the end of each request, which=
 is presumably when the worker returns control to FPM.<br></div></div></=
div></blockquote><div><br></div><div>To be honest, I haven't seen a 'slo=
w' memory leak in a long time -- except when using fgetcsv which has had=
 a reported memory leak since ~2012 somewhere on the old bug tracker. (I=
 lost the link to it and don't even remember how to get to the tracker o=
r if it still exists.) I haven't checked if it has been fixed since 8.2,=
 but I've seen a couple of reports of it on r/php a couple of times in t=
he last couple of years.<br></div><div><br></div><div id=3D"sig121229152=
">=E2=80=94 Rob<br></div></body></html>
--5f8ee6bb1e224c93904d3ea73b7d8a8a--