Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:127556 X-Original-To: internals@lists.php.net Delivered-To: internals@lists.php.net Received: from php-smtp4.php.net (php-smtp4.php.net [45.112.84.5]) by lists.php.net (Postfix) with ESMTPS id AAFC11A00BC for ; Tue, 3 Jun 2025 14:24:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=php.net; s=mail; t=1748960542; bh=EgOFhqo1q+eMyfxzSzJk0LcDiLpoaBoIefE+rHfoG7c=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=NbfKGqHfr+/8fc6SSOOijb/wh5eXRKZ6d6ySXbbkjg2apg7FzYGWvwEtu+YTCXPug Ix6FyFAd/eadH6U9s6nuu73xfe4pmC/0V9SMiUIH7ZFhUi76ZxJkmBIM0Wk75rdhy4 7MDaL6TvVB/DHAoDkZCcviYcyWL2auZOco8V4vVAsC8pXu0AuD8rsM4s68wIeLxtZR uvKie4im6//6tDc1cgloAlasObWTDyh3K3KnXS8F9E8QhUrWBWeNMgKkG1cWOx6i3j X19oDaJ22kGUDhvX79quiI8YMw18en+pu1t+CpcS4GEU9HfxAct6eyr6V0iumiN6WS wVSS/uccIR96A== Received: from php-smtp4.php.net (localhost [127.0.0.1]) by php-smtp4.php.net (Postfix) with ESMTP id 4F36B180082 for ; Tue, 3 Jun 2025 14:22:21 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on php-smtp4.php.net X-Spam-Level: X-Spam-Status: No, score=-0.2 required=5.0 tests=BAYES_40,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,DMARC_PASS,FREEMAIL_FROM, FREEMAIL_REPLY,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=4.0.1 X-Spam-Virus: Error (Cannot connect to unix socket '/var/run/clamav/clamd.ctl': connect: Connection refused) X-Envelope-From: Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by php-smtp4.php.net (Postfix) with ESMTPS for ; Tue, 3 Jun 2025 14:22:21 +0000 (UTC) Received: by mail-lj1-f172.google.com with SMTP id 38308e7fff4ca-32a6fc6f1e2so27153841fa.1 for ; Tue, 03 Jun 2025 07:24:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1748960664; x=1749565464; darn=lists.php.net; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Wb0j6K5jaZfr591PBsXpMVxKMAjcTnOIdMaKpNRLEe8=; b=a/Rz2uW7CK+ZLWKjx4durkAfe5ioWe8hoTigbLZhOztiAr+5WlWTevquI/mGAsAqaj 85KgyKTzuDQjGnvapyuMUEO7ZcOzf8lEtN4XcKr/BfNoJk8NHOPP1zGR3wAvgyyBtcAu 1PqwbnrZG+YmfT3eVuUaQu2FPby+PCnhAo7UzpehUHstHUzvRP8VrhTsnqyR2jUB88mf bflxd4j6noM6Xixslg9MmZd//0diVrax9XjHAhH77XZ5GKVpqLuePiciAKXxHFR2r/Qv r16GS6nBAZT1VtjY22kuZyX8KEXgspiXafNQnsFiJW0CplE8VbXgexnbbWH71ghRUwwa NAVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1748960664; x=1749565464; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Wb0j6K5jaZfr591PBsXpMVxKMAjcTnOIdMaKpNRLEe8=; b=qJg9FgcgW5njIebdmQp3fhtyAEs2lEZLLW1EznxUxMQ308Z4w8HGFH3/lo5QDCASfe 5KR7T6Ow1s/+dj02Ro96lPgURAb0b7OcutlxI5zvFYUOaV92j8IlwI+wDTJcSl/KVIG9 fBRv5wZZQKWxf2TdvZcxiUOngzmRFvDj0cU2kCJQFkpD5nuh/s37PyB+lh8giZv+zaL1 zI6/7SDmUeYnf7aM65L3kqxlIWQ6nOM6kM+2m+kpi0QKaAbJRUUTiF77hNZvWo4wzMYb BaJegouwj4kXnMYdeyvaDS36GbF5eT/miVogbZrte8TFcjXwNWkz18CR0rquful5/Svb 1GaQ== X-Gm-Message-State: AOJu0YzfvaBCPPoXKB4GyOKdtKRkuwq5E8KtFSK1yezkLITSstqVR7pG fSRZkM6CbcD/4CoOdoPc8Mw4T8J4Qaa7pWnHhUFyEKKRvP1zWY4cUjTaSFat+bCP0WdXGMLb5Jz 6mbQ6hLN3mWf1BppnOWcoWvofQYMaU8NnisQaIFQ= X-Gm-Gg: ASbGnctaLq6pBWHPg/Bioma42jrVDZCpg/gilXV4h/HBl+k9mKj2qxCacQJ1Zkho3+R +qSHc7Aq57VQWawqfBKM6Z5XgQwttxumiYRWUHUX/kZ0iqC2KgGB0w2c6OftT9iwdyvMOQLd1vw GHDOdXKDkCWcJCfrEml41e+//zPhrOxYtuYj3Wx0t7eyp5eZi8M0TWog== X-Google-Smtp-Source: AGHT+IE7iuAENI+5mTVhVmqhvLuEYUWi/zc0SAlw/dx7OrHWnVb0frHIzexJmp9I6cfaQ1YHxF+bMXDUnvN27b+dubM= X-Received: by 2002:a2e:be23:0:b0:329:17b0:f45c with SMTP id 38308e7fff4ca-32a9ea76a1cmr33558851fa.29.1748960663290; Tue, 03 Jun 2025 07:24:23 -0700 (PDT) Precedence: bulk list-help: list-post: List-Id: internals.lists.php.net x-ms-reactions: disallow MIME-Version: 1.0 References: In-Reply-To: Date: Tue, 3 Jun 2025 16:24:11 +0200 X-Gm-Features: AX0GCFu0b6zeI2lJ5Vk7_lxjb21ofy_edZBjF8yRzmomnhY29xxwXUZXFhwROSw Message-ID: Subject: Re: [PHP-DEV] Re: [RFC] Clone with v2 To: Volker Dusch Cc: php internals , =?UTF-8?Q?Tim_D=C3=BCsterhus?= Content-Type: multipart/alternative; boundary="0000000000000fc9d90636aba325" From: nicolas.grekas+php@gmail.com (Nicolas Grekas) --0000000000000fc9d90636aba325 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Volker, Sorry for the delay in answering, it's been a long week-end AFK on my side. Le mer. 28 mai 2025 =C3=A0 16:52, Volker Dusch a =C3=A9crit : > Hi Nicolas, > > Thank you for the great email and for thinking this through. Getting inpu= t > from the folks that maybe will use this feature the most is very valuable= , > and I appreciate that you're taking the time to do this early. > > On Mon, May 26, 2025 at 4:37=E2=80=AFPM Nicolas Grekas < > nicolas.grekas+php@gmail.com> wrote: > >> - To me, the main technical drawback of the RFC is that it builds on >> mandatory deep-cloning, which means wasted CPU/mem resources by design. >> This has to be mentioned in the RFC IMHO so that voters can make an >> informed decision. >> >> About this last point, it's the one that makes me undecided about my >> voting decision on the RFC. >> I shared my concern and a solution in another thread, where I also >> address the BC concern that is mentioned in the RFC. Since this concern = has >> been addressed, I think this should be considered more closely. As is, >> that's a big omission to me - recognizing and addressing the issue with >> deep-cloning. >> > > Quoting from your other email: > > > Allow to skip deep-cloning of already updated properties (that's a > significant drawback of the current proposal - deep cloning before settin= g > is a potential perf/mem hog built into the engine) : guarding deep-clonin= g > with a strict comparison would be enough. > > To make sure we're on the same page: This would only come into effect if > both a __clone method exists and clone($thing, $withProperties) is used. > Correct. - We decided to __clone before updating properties to avoid BC issues. > Not sure which BC issues you've in mind, especially as that's a new feature= . As I see it, before or after wouldn't change anything as far as __clone implementations are concerned. > - For readonly classes, given their dependencies are likely to be readonl= y > as well, they don't need to be deep cloned in most cases? > - Therefore, I think combining __clone and clone-with will be a rare > occasion, but all performance issues can be avoided by only using one of > the two approaches in classes where that matters. That's a very common mistake about readonly: it's also very useful for mutable classes. Here is an example that the Symfony Request/Response objects might follow one day: class Response { public function __construct( public string $content =3D '', public *readonly* ArrayObject $headers =3D new ArrayObject(), ) { } public function __clone() { $this->headers =3D clone $this->headers; } } $r1 =3D new Response(); $r2 =3D clone $r1; $r3 =3D clone($r1, ['headers' =3D> new ArrayObject(['foo' =3D> 'bar'])]); Note how making "headers" readonly binds the headers to its owner object. That's very useful since it forbids swapping the headers object by another one in response objects. I don't think this is/will be/should be a rare design, nor do I think this should be ignored in the design of the proposed RFC. The issue I highlighted previously is that __clone will "clone $this->headers" even if the resulting value will be immediately trashed on the line that creates $r3. This can cascade to nested clones of course depending on what is being cloned. That's where I see a waste of CPU/mem "by-design", which is unavoidable at the moment. - This RFC leaves future modifications to __clone open, so we could discuss > a follow-up of passing the properties to __clone if we have a strategy to > handle the BC issues, but for now, I see this as adding too much complexi= ty > for a very limited set of use cases that already are solved by the people > that have them. > About the BC aspect I already shared a proposal that should avoid it. About the "very limited set of use cases", that's simply not true: people will start using the new syntax when consuming third-party classes, and authors of those classes will be "Sorry Outta Luck" when their users will start complaining about the perf hog I described, with no other options than telling "don't use that syntax, that perfhog is built-in"... > So no situation will be worse than it was before, while some use-cases ar= e > enabled now and some performance critical code will probably stay the sam= e > as it is now. > > > - writing code with a wide range of supported PHP versions means we'll > start using construct such as: > > if (PHP_VERSION_ID>=3D80500) { > > $b =3D 'clone'($a, [...]); // note 'clone' is a string here, so that = the > line is valid syntax on PHP <=3D 8.4 > > else { > > // another code path for PHP 8.4 > > } > > Tim has already addressed the \namespace fallback and that this doesn't > apply here, and that no performance penalty is introduced into any existi= ng > codebases. > So my personal suggestion would to be for code that is doing custom things > during cloning and support PHP pre-clone-with as is and only refactor onc= e > 8.5 is the minimum requirement, in places where it improves the code. > I didn't answer Tim but he missed that in my example there is a second argument passed to clone(), which makes it a parse error currently. About the second aspect: if a class is designed with the API I gave as example above (that Response class), then the new syntax is going to be the only way to swap the headers while cloning. That means there will be cases where my snippet is going to be useful. I'm fine if you don't want to add it to the RFC of course - at least there's this thread for history and this could be documented later on somewhere else. Note that the new syntax being the only way to swap the headers while cloning means that all similarly-designed classes will *have to* live by the deep-cloning issue. As a corollary, authors that will want to avoid the deep-cloning issue will be forced to skip such designs. And that's a shame because that design looks desirable to me (and authors might realize the issue after-the-fact, which is even worse...) So what remains on my side is giving enough care to the deep-cloning case - see my previous messages of course. Nicolas --0000000000000fc9d90636aba325 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Volker,

= Sorry for the delay in answering, it's been a long week-end AFK on my s= ide.

Le=C2=A0mer. 28 mai 2025 =C3=A0=C2=A016:52,= Volker Dusch <volker@tidewa= ys-gmbh.com> a =C3=A9crit=C2=A0:
Hi=C2=A0Nicolas,

Thank you for the great email and for thinking thi= s through. Getting input from the folks that maybe will use this feature th= e most is very valuable, and I appreciate that you're taking=C2=A0the t= ime to do this early.

On M= on, May 26, 2025 at 4:37=E2=80=AFPM Nicolas Grekas <nicolas.grekas+php@gmail.co= m> wrote:
- = To me, the main technical drawback of the RFC is that it builds on mandator= y deep-cloning, which means wasted CPU/mem resources by design. This has t= o be mentioned in the RFC IMHO so that voters can make an informed decision= .

About this last point, it's the one that mak= es me undecided about my voting decision on the RFC.
I shared my = concern and a solution in another thread, where I also address the BC conce= rn that is mentioned in the RFC. Since this concern has been addressed, I t= hink this should be considered more closely. As is, that's a big omissi= on to me - recognizing and addressing the issue with deep-cloning.

Quoting from your other email:

> Allow to skip deep-cloning of already updated p= roperties (that's a significant drawback of the current proposal - deep= cloning before setting is a potential perf/mem hog built into the engine) = : guarding deep-cloning with a strict comparison would be enough.
=C2=A0
To make sure we're on the same page: This would only = come into effect if both a __clone method exists and clone($thing, $withPro= perties) is used.

Correct= .=C2=A0

- We decided to __clone= before updating properties to avoid BC issues.

Not sure which BC issues you've in mind, especia= lly as that's a new feature.
As I see it, before or after wou= ldn't change anything as far as __clone implementations are concerned.<= /div>
=C2=A0
- For readonly classes, given= their dependencies are likely to be readonly as well, they don't need = to be deep cloned in most cases?
- Therefore, I think combining _= _clone and clone-with will be a=20 rare occasion, but all performance issues can be avoided by only using=20 one of the two approaches in classes where that matters.
<= br>
That's a very common mistake about readonly: it'= ;s also very useful for mutable classes. Here is an example that the Symfon= y Request/Response objects might follow one day:

class = Response
{
=C2=A0 =C2=A0 public function __construct(
=C2=A0 =C2= =A0 =C2=A0 =C2=A0 public string $content =3D '',
=C2=A0 =C2=A0 = =C2=A0 =C2=A0 public readonly ArrayObject $headers =3D new ArrayObje= ct(),
=C2=A0 =C2=A0 ) {
=C2=A0 =C2=A0 }

=C2=A0 =C2=A0 public f= unction __clone()=C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0 $this->headers = =3D clone $this->headers;
=C2=A0 =C2=A0 }
}

$r1 =3D new Res= ponse();
$r2 =3D clone $r1;
$r3 =3D clone($r1, ['headers' =3D= > new ArrayObject(['foo' =3D> 'bar'])]);
=C2=A0
Note how making "headers" readonly binds the headers to = its owner object. That's very useful since it forbids=C2=A0swapping the= headers object by another one in response objects.

I don't think this is/will be/should be a rare design, nor do I think= this should be ignored in the design of the proposed RFC.

Th= e issue I highlighted previously is that __clone will "clone $this->= ;headers" even if the resulting value will be immediately trashed on t= he line that creates $r3. This can cascade to nested clones of course depen= ding on what is being cloned. That's where I see a waste of CPU/mem &qu= ot;by-design", which is unavoidable at the moment.

- This RFC leaves future modifications=C2=A0to __c= lone open, so we could discuss a follow-up of passing the properties to __c= lone if we have a strategy to handle the BC issues, but for now, I see this= as adding too much complexity for a very limited set of use cases that=C2= =A0already are solved by the people that have them.=C2=A0
=

About the BC aspect I already shared a pro= posal that should avoid it. About the "very limited set of use cases&q= uot;, that's simply not true: people will start using the new syntax wh= en consuming third-party classes, and authors of those classes will be &quo= t;Sorry Outta Luck" when their users will start complaining about the = perf hog=C2=A0I described, with no other options than telling "don'= ;t use that syntax, that perfhog is built-in"...
=C2=A0
So no situation will be worse than it was before, while some use-c= ases are enabled now and some performance critical code will probably=C2=A0= stay the same as it is now.

> - writing code wi= th a wide range of supported PHP versions means we'll start using const= ruct such as:
> if (PHP_VERSION_ID>=3D80500) {
&g= t;=C2=A0 =C2=A0$b =3D 'clone'($a, [...]); // note 'clone' i= s a string here, so that the line is valid syntax on PHP <=3D 8.4
<= div>> else {
>=C2=A0 =C2=A0// another code path for PHP 8.4=
> }

Tim has already addressed the \n= amespace fallback and that this doesn't apply here, and that no perform= ance penalty is introduced into any existing codebases.=C2=A0
So my personal suggestion would to be for code that is d= oing custom things during cloning and support PHP pre-clone-with as is and = only refactor once 8.5 is the minimum requirement,=C2=A0in places where it = improves the code.=C2=A0

I= didn't answer Tim but he missed that in my example there is a second a= rgument passed to clone(), which makes it a parse error currently.

About the second aspect: if a class is designed with the A= PI I gave as example above (that Response class), then the new syntax is go= ing to be the only way to swap the headers while cloning. That means there = will be cases where my snippet is going to be useful. I'm fine if you d= on't want to add it to the RFC of course - at least there's this th= read for history and this could be documented later on somewhere else.
Note that the new syntax being the only way to swap the headers while= cloning means that all similarly-designed classes will *have to* live by t= he deep-cloning issue. As a corollary, authors that will want to avoid the = deep-cloning issue will be forced to skip such designs. And that's a sh= ame because that design looks desirable to me (and authors might realize th= e issue after-the-fact, which is even worse...)

So= what remains on my side is giving enough care to the deep-cloning case - s= ee my previous messages of course.

Nicolas
--0000000000000fc9d90636aba325--