Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123517
Precedence: bulk
MIME-Version: 1.0
References: <CAOWwgpmbq5VRrZQvUXDsKiNK4r6+bFA4VxnjQ_U=h8T9r0o3DA@mail.gmail.com>
 <ab83af79-0669-47dd-a3cb-ab72327ae174@bastelstu.be>
In-Reply-To: <ab83af79-0669-47dd-a3cb-ab72327ae174@bastelstu.be>
Date: Wed, 5 Jun 2024 16:50:21 +0200
Message-ID: <CAP1Jc13JWVg99AULgzrGXcoCWJtJZGz+nr_VoyUocw8n5n5sfQ@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] Lazy Objects
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>
Cc: Nicolas Grekas <nicolas.grekas+php@gmail.com>, 
	PHP Internals List <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: arnaud.lb@gmail.com (Arnaud Le Blanc)

Hi Tim,

That's a lot of interesting feedback. I will try to answer some of
your points, and Nicolas will follow with other points.

On Tue, Jun 4, 2024 at 9:16=E2=80=AFPM Tim D=C3=BCsterhus <tim@bastelstu.be=
> wrote:
> - int $options =3D 0
>
> Not a fan of flag parameters that take a bitset, those provide for a
> terrible DX due to magic numbers. Perhaps make this a regular (named)
> parameter, or an list of enum LazyObjectOptions { case
> SkipInitOnSerialize; }?

The primary reason for choosing to represent $options as a bitset is
that it's consistent with the rest of the Reflection API (e.g.
ReflectionClass::getProperties() uses a bitset for the $filter
parameter).

I don't get your point about magic numbers since we are using
constants to abstract them.

> - skipProperty()
>
> Not a fan of the method name, because it doesn't really say what it
> does, without consulting the docs. Perhaps `skipInitializationFor()` or
> similar?

We have opted for skipInitializerForProperty()

> - setProperty()
>
> Not a fan of the method name, because it is not a direct counterpart to
> `getProperty()`. Unfortunately I don't have a better suggestion.

We have renamed setRawProperty() to setRawPropertyValue() in the RFC.
We are open to other suggestions.

We have also removed setProperty(), as we believe that there is no
use-case for it.

> - The examples should be expanded and clarified, especially the one for
> makeLazyProxy():

Agreed. We will add examples and clarify some behaviors.

> My understanding is that the $object that is passed to the first
> parameter of makeLazyProxy() is completely replaced. Is this
> understanding correct? What does that mean for spl_object_hash(),
> spl_object_id()? What does this mean for WeakMap and WeakReference? What
> does this mean for objects that are only referenced from within $object?

The object is updated in-place, and retains its identity. It is not
replaced. What makeLazyGhost() and makeLazyProxy() do is equivalent to
calling `unset()` on all properties, and setting a flag on the object
internally. Apart from setting the internal flag, this is achievable
in userland by iterating on all properties via the Reflection API, and
using unset() in the right scope with a Closure.

spl_object_id(), spl_object_hash(), SplObjectStorage, WeakMap,
WeakReference, strict equality, etc are not affected by makeLazy*().

The intended use of makeLazyGhost() and makeLazyProxy() is to call
them either on an object created with
ReflectionClass::newInstanceWithoutConstructor(), or on $this in a
constructor. The latter is the reason why these APIs take an existing
object.

The proposed patch integrates into the object handlers fallback code
path used to manage accesses to undefined properties. We implement
lazy initialization by hooking into undefined property accesses,
without impacting the fast path.

> Consider this example:
>
>      class Foo {
>        public function __destruct() { echo __METHOD__; }
>      }
>
>      class Bar {
>        public string $s;
>        public ?Foo $foo;
>
>       public function __destruct() { echo __METHOD__; }
>      }
>
>      $bar =3D new Bar();
>      $bar->foo =3D new Foo();
>
>      ReflectionLazyObject::makeLazyProxy($bar, function (Bar $bar) {
>        $result =3D new Bar();
>        $result->foo =3D null;
>        $result->s =3D 'init';
>        return $result;
>      });
>
>      var_dump($bar->s);
>
> My understanding is that this will dump `string(4) "init"`. Will the
> destructor of Foo be called? Will the destructor of Bar be called?

This will print:

    Foo::__destruct (during makeLazyProxy())
    string(4) "init" (during var_dump())

and eventually

    Bar::__destruct (when $bar is released)

> - What happens if I make an object lazy that already has all properties
> initialized? Will that be a noop? Will that throw? Will that create a
> lazy object that will never automatically be initialized?

All properties are unset as described earlier, and the object is
flagged as lazy. The object will automatically initialize when trying
to observe its properties.

However, making a fully initialized object lazy is not the intended use-cas=
e.

> - Cloning, unless __clone() is implemented and accesses a property.
>
> The semantics of cloning a lazy object should be explicitly spelled out
> in the RFC, ideally with an example of the various edge cases (should
> any exist).

Agreed. We are working on expanding the RFC about this

> - Before calling the initializer, properties that were not initialized
> with ReflectionLazyObject::skipProperty(),
> ReflectionLazyObject::setProperty(),
> ReflectionLazyObject::setRawProperty() are initialized to their default
> value.
>
> Should skipProperty() also skip the initialization to the default value?
> My understanding is that it allows skipping the initialization on
> access, but when initialization actually happens it should probably be
> set to a well-defined value, no?
>
> Am I also correct in my understanding that this should read "initialized
> to their default value (if any)", meaning that properties without a
> default value are left uninitialized?

The primary effect of skipProperty() is to mark a property as
non-lazy, so that accessing it does not trigger the initialization of
the entire object. It also sets the property to its default value if
any, otherwise it is left as undef.

Accessing this property afterwards has exactly the same effect as
doing so on an object created with
ReflectionClass::newInstanceWithoutConstructor() (including triggering
errors when reading an uninitialized property).

> - If an exception is thrown while calling the initializer, the object is
> reverted to its pre-initialization state and is still considered lazy.
>
> Does this mean that the initializer will be called once again when
> accessing another property?

Yes. The goal is to prevent transition from lazy to initialized when
an unexpected error occurred in the initializer.

> Will the "revert to its pre-initialization"
> work properly when you have nested lazy objects? An example would
> probably help.

Only the effects on the object itself are reverted. External side
effects are not reverted.

> - The initializer is called with the object as first parameter.
>
> What is the behavior of accessing the object properties, while the
> initializer is active? Based on the examples, I assume it will not be
> recursively called, similarly to how the hooks work?

For ghost objects, the initializer is supposed to initialize the
object itself like a constructor would do. During initializer
execution, the object has exactly the same state and behavior as it
would have in its constructor during `new`:

- The object is not lazy.
- Properties have their default value (if any) and are accessed
without triggering a nested initialization.
- If setRawPropertyValue() was used, some properties may have a value
different from their default.

For virtual proxies, the initializer is supposed to return another
object. Accessing the proxy object during initialization does not
trigger recursive initializations. Some properties will have a value
if setRawPropertyValue() or skipInitializationForProperty().

> - The object is marked as non-lazy and the initializer is released.
>
> What does it mean for the initializer to be released? Consider the
> following example:
>
>      ReflectionLazyObject::makeLazyGhost($o, $init =3D function ($o) use
> (&$init) {
>        $o->init =3D $init;
>      });

"released" here means that the initializer is not referenced anymore
by this object, and may be freed if it is not referenced anywhere
else.

> - The return value of the initializer has to be an instance of a parent
> or a child class of the lazy-object and it must have the same properties.
>
> Would returning a parent class not violate the LSP? Consider the
> following example:
>
>      class A { public string $s; }
>      class B extends A { public function foo() { } }
>
>      $o =3D new B();
>      ReflectionLazyObject::makeLazyProxy($o, function (B $o) {
>        return new A();
>      });
>
>      $o->foo(); // works
>      $o->s =3D 'init';
>      $o->foo(); // breaks

$o->foo() calls B::foo() in both cases here, as $o is always the proxy
object. We need to double check, but we believe that this rule doesn't
break LSP.

> - The destructor of lazy non-initialized objects is not called.
>
> That sounds unsafe. Consider the following example:
>
>      class Mutex {
>        public string $s;
>        public function __construct() {
>          // take lock
>        }
>
>        public function __destruct() {
>          // release lock
>        }
>      }
>
>      $m =3D new Mutex();
>      ReflectionLazyObject::makeLazyGhost($m, function ($m) {
>      });
>      unset($m); // will not release the lock.

Good point. The Mutex constructor is called during "new Mutex()", but
the object is made lazy after that, and the destructor is never
called.

We have made the following changes to the RFC:

- makeLazyGhost / makeLazyProxy will call the object destructor
- A new option flag is added, `ReflectionLazyObject::SKIP_DESTRUCTOR`,
that disables this behavior

This is not ideal since the intended use of these methods is to call
them on objects created with newInstanceWithoutConstructor(), or
directly in a constructor, and both of these will need this flag, but
at least it's safe by default.

Thanks again for the feedback.

Best Regards,
Arnaud